[PATCH v1 0/3] fixing building errors and warnings when components

[PATCH v1 0/3] fixing building errors and warnings when components

[Yaniv Gardi]

V1:
The following combination of components, when SCSI_UFS_QCOM=y
and PHY_QCOM_UFS=m is illegal and causes build errors.
The 3rd patch in the series enables the SCSI_UFS_QCOM component to 
be compiled as a module (by changing its configuration to tristate).
So now, compiling SCSI_UFS_QCOM=m forces PHY_QCOM_UFS=m, and 
SCSI_UFS_QCOM=y forces PHY_QCOM_UFS=y.

In addition, when PHY_QCOM_UFS=m, external functions in 
phy-ufs-qcom.c should be exported. The 1st patch fixes it.

Another issue that we see when SCSI_UFS_QCOM=m is a warning that
the 2nd patch fixes.

notice:
checkpatch gives an error on the commit message of patch 1/3
in the series. Ignore as the commit message is the build errors
that this patch fixes.

Yaniv Gardi (3):
  phy: qcom-ufs: fix build error when the driver is built as a module
  scsi: ufs-qcom: fix compilation warning if compiled as a module
  scsi: ufs-qcom: update configuration option of SCSI_UFS_QCOM component

 drivers/phy/phy-qcom-ufs.c  | 11 +++++++++++
 drivers/scsi/ufs/Kconfig    |  2 +-
 drivers/scsi/ufs/ufs-qcom.c |  2 +-
 3 files changed, 13 insertions(+), 2 deletions(-)

-- 
1.8.5.2

-- 
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v1 1/3] phy: qcom-ufs: fix build error when the driver is built as a module

[Yaniv Gardi]

Export the following functions in order to avoid build errors
when the driver is compiled as a module:

ERROR: "ufs_qcom_phy_disable_ref_clk" [drivers/scsi/ufs/ufs-qcom.ko] undefined!
ERROR: "ufs_qcom_phy_enable_ref_clk" [drivers/scsi/ufs/ufs-qcom.ko] undefined!
ERROR: "ufs_qcom_phy_is_pcs_ready" [drivers/scsi/ufs/ufs-qcom.ko] undefined!
ERROR: "ufs_qcom_phy_disable_iface_clk" [drivers/scsi/ufs/ufs-qcom.ko] undefined!
ERROR: "ufs_qcom_phy_start_serdes" [drivers/scsi/ufs/ufs-qcom.ko] undefined!
ERROR: "ufs_qcom_phy_calibrate_phy" [drivers/scsi/ufs/ufs-qcom.ko] undefined!
ERROR: "ufs_qcom_phy_enable_dev_ref_clk" [drivers/scsi/ufs/ufs-qcom.ko] undefined!
ERROR: "ufs_qcom_phy_set_tx_lane_enable" [drivers/scsi/ufs/ufs-qcom.ko] undefined!
ERROR: "ufs_qcom_phy_disable_dev_ref_clk" [drivers/scsi/ufs/ufs-qcom.ko] undefined!
ERROR: "ufs_qcom_phy_save_controller_version" [drivers/scsi/ufs/ufs-qcom.ko] undefined!
ERROR: "ufs_qcom_phy_enable_iface_clk" [drivers/scsi/ufs/ufs-qcom.ko] undefined!
make[1]: *** [__modpost] Error 1

Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>

---
 drivers/phy/phy-qcom-ufs.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/phy/phy-qcom-ufs.c b/drivers/phy/phy-qcom-ufs.c
index f9c618f..6140a8b 100644
--- a/drivers/phy/phy-qcom-ufs.c
+++ b/drivers/phy/phy-qcom-ufs.c
@@ -432,6 +432,7 @@ out_disable_src:
 out:
 	return ret;
 }
+EXPORT_SYMBOL_GPL(ufs_qcom_phy_enable_ref_clk);
 
 static
 int ufs_qcom_phy_disable_vreg(struct phy *phy,
@@ -474,6 +475,7 @@ void ufs_qcom_phy_disable_ref_clk(struct phy *generic_phy)
 		phy->is_ref_clk_enabled = false;
 	}
 }
+EXPORT_SYMBOL_GPL(ufs_qcom_phy_disable_ref_clk);
 
 #define UFS_REF_CLK_EN	(1 << 5)
 
@@ -517,11 +519,13 @@ void ufs_qcom_phy_enable_dev_ref_clk(struct phy *generic_phy)
 {
 	ufs_qcom_phy_dev_ref_clk_ctrl(generic_phy, true);
 }
+EXPORT_SYMBOL_GPL(ufs_qcom_phy_enable_dev_ref_clk);
 
 void ufs_qcom_phy_disable_dev_ref_clk(struct phy *generic_phy)
 {
 	ufs_qcom_phy_dev_ref_clk_ctrl(generic_phy, false);
 }
+EXPORT_SYMBOL_GPL(ufs_qcom_phy_disable_dev_ref_clk);
 
 /* Turn ON M-PHY RMMI interface clocks */
 int ufs_qcom_phy_enable_iface_clk(struct phy *generic_phy)
@@ -550,6 +554,7 @@ int ufs_qcom_phy_enable_iface_clk(struct phy *generic_phy)
 out:
 	return ret;
 }
+EXPORT_SYMBOL_GPL(ufs_qcom_phy_enable_iface_clk);
 
 /* Turn OFF M-PHY RMMI interface clocks */
 void ufs_qcom_phy_disable_iface_clk(struct phy *generic_phy)
@@ -562,6 +567,7 @@ void ufs_qcom_phy_disable_iface_clk(struct phy *generic_phy)
 		phy->is_iface_clk_enabled = false;
 	}
 }
+EXPORT_SYMBOL_GPL(ufs_qcom_phy_disable_iface_clk);
 
 int ufs_qcom_phy_start_serdes(struct phy *generic_phy)
 {
@@ -578,6 +584,7 @@ int ufs_qcom_phy_start_serdes(struct phy *generic_phy)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(ufs_qcom_phy_start_serdes);
 
 int ufs_qcom_phy_set_tx_lane_enable(struct phy *generic_phy, u32 tx_lanes)
 {
@@ -595,6 +602,7 @@ int ufs_qcom_phy_set_tx_lane_enable(struct phy *generic_phy, u32 tx_lanes)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(ufs_qcom_phy_set_tx_lane_enable);
 
 void ufs_qcom_phy_save_controller_version(struct phy *generic_phy,
 					  u8 major, u16 minor, u16 step)
@@ -605,6 +613,7 @@ void ufs_qcom_phy_save_controller_version(struct phy *generic_phy,
 	ufs_qcom_phy->host_ctrl_rev_minor = minor;
 	ufs_qcom_phy->host_ctrl_rev_step = step;
 }
+EXPORT_SYMBOL_GPL(ufs_qcom_phy_save_controller_version);
 
 int ufs_qcom_phy_calibrate_phy(struct phy *generic_phy, bool is_rate_B)
 {
@@ -625,6 +634,7 @@ int ufs_qcom_phy_calibrate_phy(struct phy *generic_phy, bool is_rate_B)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(ufs_qcom_phy_calibrate_phy);
 
 int ufs_qcom_phy_remove(struct phy *generic_phy,
 			struct ufs_qcom_phy *ufs_qcom_phy)
@@ -662,6 +672,7 @@ int ufs_qcom_phy_is_pcs_ready(struct phy *generic_phy)
 	return ufs_qcom_phy->phy_spec_ops->
 			is_physical_coding_sublayer_ready(ufs_qcom_phy);
 }
+EXPORT_SYMBOL_GPL(ufs_qcom_phy_is_pcs_ready);
 
 int ufs_qcom_phy_power_on(struct phy *generic_phy)
 {
-- 
1.8.5.2

-- 
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v1 2/3] scsi: ufs-qcom: fix compilation warning if compiled as a module

[Yaniv Gardi]

This change fixes a compilation warning that happens if SCSI_UFS_QCOM
is compiled as a module.

Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>

---
 drivers/scsi/ufs/ufs-qcom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
index 6652a81..2e4ad21 100644
--- a/drivers/scsi/ufs/ufs-qcom.c
+++ b/drivers/scsi/ufs/ufs-qcom.c
@@ -885,7 +885,7 @@ out:
 
 #define	ANDROID_BOOT_DEV_MAX	30
 static char android_boot_dev[ANDROID_BOOT_DEV_MAX];
-static int get_android_boot_dev(char *str)
+static int __maybe_unused get_android_boot_dev(char *str)
 {
 	strlcpy(android_boot_dev, str, ANDROID_BOOT_DEV_MAX);
 	return 1;
-- 
1.8.5.2

-- 
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v1 3/3] scsi: ufs-qcom: update configuration option of SCSI_UFS_QCOM component

[Yaniv Gardi]

This change is required in order to be able to build the component
as a module.

Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>

---
 drivers/scsi/ufs/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
index e945383..5f45307 100644
--- a/drivers/scsi/ufs/Kconfig
+++ b/drivers/scsi/ufs/Kconfig
@@ -72,7 +72,7 @@ config SCSI_UFSHCD_PLATFORM
 	  If unsure, say N.
 
 config SCSI_UFS_QCOM
-	bool "QCOM specific hooks to UFS controller platform driver"
+	tristate "QCOM specific hooks to UFS controller platform driver"
 	depends on SCSI_UFSHCD_PLATFORM && ARCH_QCOM
 	select PHY_QCOM_UFS
 	help
-- 
1.8.5.2

-- 
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v1 3/3] scsi: ufs-qcom: update configuration option of SCSI_UFS_QCOM component

[Paul Bolle]

On Tue, 2015-05-19 at 16:47 +0300, Yaniv Gardi wrote:
> This change is required in order to be able to build the component
> as a module.
> 
> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>

> --- a/drivers/scsi/ufs/Kconfig
> +++ b/drivers/scsi/ufs/Kconfig
 
>  config SCSI_UFS_QCOM
> -	bool "QCOM specific hooks to UFS controller platform driver"
> +	tristate "QCOM specific hooks to UFS controller platform driver"
>  	depends on SCSI_UFSHCD_PLATFORM && ARCH_QCOM
>  	select PHY_QCOM_UFS
>  	help

As far as I can see, in next-20150519, drivers/scsi/ufs/ufs-qcom.c lacks
the required module specific boilerplate for this to be useful. Is that
boilerplate added in another series?

Thanks,


Paul Bolle

Re: [PATCH v1 1/3] phy: qcom-ufs: fix build error when the driver is built as a module

[Paul Bolle]

On Tue, 2015-05-19 at 16:47 +0300, Yaniv Gardi wrote:
> Export the following functions in order to avoid build errors
> when the driver is compiled as a module:

Where "the driver" actually means something like ufs-qcom.c, or
SCSI_UFS_QCOM, or "QCOM specific hooks to UFS controller platform
driver", right?

> ERROR: "ufs_qcom_phy_disable_ref_clk" [drivers/scsi/ufs/ufs-qcom.ko] undefined!
> ERROR: "ufs_qcom_phy_enable_ref_clk" [drivers/scsi/ufs/ufs-qcom.ko] undefined!
> ERROR: "ufs_qcom_phy_is_pcs_ready" [drivers/scsi/ufs/ufs-qcom.ko] undefined!
> ERROR: "ufs_qcom_phy_disable_iface_clk" [drivers/scsi/ufs/ufs-qcom.ko] undefined!
> ERROR: "ufs_qcom_phy_start_serdes" [drivers/scsi/ufs/ufs-qcom.ko] undefined!
> ERROR: "ufs_qcom_phy_calibrate_phy" [drivers/scsi/ufs/ufs-qcom.ko] undefined!
> ERROR: "ufs_qcom_phy_enable_dev_ref_clk" [drivers/scsi/ufs/ufs-qcom.ko] undefined!
> ERROR: "ufs_qcom_phy_set_tx_lane_enable" [drivers/scsi/ufs/ufs-qcom.ko] undefined!
> ERROR: "ufs_qcom_phy_disable_dev_ref_clk" [drivers/scsi/ufs/ufs-qcom.ko] undefined!
> ERROR: "ufs_qcom_phy_save_controller_version" [drivers/scsi/ufs/ufs-qcom.ko] undefined!
> ERROR: "ufs_qcom_phy_enable_iface_clk" [drivers/scsi/ufs/ufs-qcom.ko] undefined!
> make[1]: *** [__modpost] Error 1

Thanks,


Paul Bolle

Re: [PATCH v1 3/3] scsi: ufs-qcom: update configuration option of SCSI_UFS_QCOM component

[Paul Bolle]

On Wed, 2015-05-20 at 09:21 +0200, Paul Bolle wrote:
> As far as I can see, in next-20150519, drivers/scsi/ufs/ufs-qcom.c lacks
> the required module specific boilerplate for this to be useful. Is that
> boilerplate added in another series?

I need to rephrase this. Let me try again.

As far as I can see, in next-20150519, drivers/scsi/ufs/ufs-qcom.c lacks
a MODULE_LICENSE() macro. Without that macro loading the module should
trigger a warning and taint the kernel, right?

By the way, as far as I can see, this (new) module can only be loaded
manually (or via scripts). Is that what people want?

Thanks,


Paul Bolle

Re: [PATCH v1 2/3] scsi: ufs-qcom: fix compilation warning if compiled as a module

[Paul Bolle]

On Tue, 2015-05-19 at 16:47 +0300, Yaniv Gardi wrote:
> --- a/drivers/scsi/ufs/ufs-qcom.c
> +++ b/drivers/scsi/ufs/ufs-qcom.c
> @@ -885,7 +885,7 @@ out:
>  
>  #define	ANDROID_BOOT_DEV_MAX	30
>  static char android_boot_dev[ANDROID_BOOT_DEV_MAX];
> -static int get_android_boot_dev(char *str)
> +static int __maybe_unused get_android_boot_dev(char *str)
>  {
>  	strlcpy(android_boot_dev, str, ANDROID_BOOT_DEV_MAX);
>  	return 1;

Wouldn't it be clearer to wrap these few lines (until after the
__setup() macro) with #ifndef MODULE?

And I think get_android_boot_dev() could be marked __init. Because if
it's built-in it will never be called after the kernel has finished
booting, right?


Paul Bolle

Re: [PATCH v1 1/3] phy: qcom-ufs: fix build error when the driver is built as a module

[ygardi]

> On Tue, 2015-05-19 at 16:47 +0300, Yaniv Gardi wrote:
>> Export the following functions in order to avoid build errors
>> when the driver is compiled as a module:
>
> Where "the driver" actually means something like ufs-qcom.c, or
> SCSI_UFS_QCOM, or "QCOM specific hooks to UFS controller platform
> driver", right?
>

yes, Paul, you are correct.

>> ERROR: "ufs_qcom_phy_disable_ref_clk" [drivers/scsi/ufs/ufs-qcom.ko]
>> undefined!
>> ERROR: "ufs_qcom_phy_enable_ref_clk" [drivers/scsi/ufs/ufs-qcom.ko]
>> undefined!
>> ERROR: "ufs_qcom_phy_is_pcs_ready" [drivers/scsi/ufs/ufs-qcom.ko]
>> undefined!
>> ERROR: "ufs_qcom_phy_disable_iface_clk" [drivers/scsi/ufs/ufs-qcom.ko]
>> undefined!
>> ERROR: "ufs_qcom_phy_start_serdes" [drivers/scsi/ufs/ufs-qcom.ko]
>> undefined!
>> ERROR: "ufs_qcom_phy_calibrate_phy" [drivers/scsi/ufs/ufs-qcom.ko]
>> undefined!
>> ERROR: "ufs_qcom_phy_enable_dev_ref_clk" [drivers/scsi/ufs/ufs-qcom.ko]
>> undefined!
>> ERROR: "ufs_qcom_phy_set_tx_lane_enable" [drivers/scsi/ufs/ufs-qcom.ko]
>> undefined!
>> ERROR: "ufs_qcom_phy_disable_dev_ref_clk" [drivers/scsi/ufs/ufs-qcom.ko]
>> undefined!
>> ERROR: "ufs_qcom_phy_save_controller_version"
>> [drivers/scsi/ufs/ufs-qcom.ko] undefined!
>> ERROR: "ufs_qcom_phy_enable_iface_clk" [drivers/scsi/ufs/ufs-qcom.ko]
>> undefined!
>> make[1]: *** [__modpost] Error 1
>
> Thanks,
>
>
> Paul Bolle
>
>

Re: [PATCH v1 2/3] scsi: ufs-qcom: fix compilation warning if compiled as a module

[ygardi]

> On Tue, 2015-05-19 at 16:47 +0300, Yaniv Gardi wrote:
>> --- a/drivers/scsi/ufs/ufs-qcom.c
>> +++ b/drivers/scsi/ufs/ufs-qcom.c
>> @@ -885,7 +885,7 @@ out:
>>
>>  #define	ANDROID_BOOT_DEV_MAX	30
>>  static char android_boot_dev[ANDROID_BOOT_DEV_MAX];
>> -static int get_android_boot_dev(char *str)
>> +static int __maybe_unused get_android_boot_dev(char *str)
>>  {
>>  	strlcpy(android_boot_dev, str, ANDROID_BOOT_DEV_MAX);
>>  	return 1;
>
> Wouldn't it be clearer to wrap these few lines (until after the
> __setup() macro) with #ifndef MODULE?
>
> And I think get_android_boot_dev() could be marked __init. Because if
> it's built-in it will never be called after the kernel has finished
> booting, right?
>

That's correct. I will upload an update to this patch.
I accept both comments
#ifndef MODULE is indeed clearer
and __init flag as well, since get_android_boot_dev() will not be called
again after booting finished.

>
> Paul Bolle
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v1 3/3] scsi: ufs-qcom: update configuration option of SCSI_UFS_QCOM component

[Paul Bolle]

On Wed, 2015-05-20 at 10:22 +0200, Paul Bolle wrote:
> By the way, as far as I can see, this (new) module can only be loaded
> manually (or via scripts). Is that what people want?

This comment wasn't well thought through. So I hand another look at the
code of usf-qcom.

I noticed that the single thing ufs-qcom exports is "struct
ufs_hba_qcom_vops". But that's unused in next-20150520:
    $ git grep -nw ufs_hba_qcom_vops
    drivers/scsi/ufs/ufs-qcom.c:999: * struct ufs_hba_qcom_vops - UFS QCOM specific variant operations
    drivers/scsi/ufs/ufs-qcom.c:1004:static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
    drivers/scsi/ufs/ufs-qcom.c:1016:EXPORT_SYMBOL(ufs_hba_qcom_vops);

So it's not used by code outside of ufs-qcom.c. Probably because it
can't actually be used by outside code. It's not mentioned in any public
header and it's even static!

Am I missing something obvious here? Because ufs-qcom currently looks
pointless to me, and I actually see little reason to even have it in the
mainline tree. 


Paul Bolle

Re: [PATCH v1 3/3] scsi: ufs-qcom: update configuration option of SCSI_UFS_QCOM component

[ygardi]

> On Wed, 2015-05-20 at 10:22 +0200, Paul Bolle wrote:
>> By the way, as far as I can see, this (new) module can only be loaded
>> manually (or via scripts). Is that what people want?
>
> This comment wasn't well thought through. So I hand another look at the
> code of usf-qcom.
>
> I noticed that the single thing ufs-qcom exports is "struct
> ufs_hba_qcom_vops". But that's unused in next-20150520:
>     $ git grep -nw ufs_hba_qcom_vops
>     drivers/scsi/ufs/ufs-qcom.c:999: * struct ufs_hba_qcom_vops - UFS QCOM
> specific variant operations
>     drivers/scsi/ufs/ufs-qcom.c:1004:static const struct
> ufs_hba_variant_ops ufs_hba_qcom_vops = {
>     drivers/scsi/ufs/ufs-qcom.c:1016:EXPORT_SYMBOL(ufs_hba_qcom_vops);
>
> So it's not used by code outside of ufs-qcom.c. Probably because it
> can't actually be used by outside code. It's not mentioned in any public
> header and it's even static!
>
> Am I missing something obvious here? Because ufs-qcom currently looks
> pointless to me, and I actually see little reason to even have it in the
> mainline tree.
>

we haven't uploaded yet the patch that binds qcom vops to the driver, but
we will soon.

>
> Paul Bolle
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v1 3/3] scsi: ufs-qcom: update configuration option of SCSI_UFS_QCOM component

[Paul Bolle]

On Thu, 2015-05-21 at 10:09 +0000, ygardi@codeaurora.org wrote:
> > On Wed, 2015-05-20 at 10:22 +0200, Paul Bolle wrote:
> > Am I missing something obvious here? Because ufs-qcom currently looks
> > pointless to me, and I actually see little reason to even have it in the
> > mainline tree.
> >
> 
> we haven't uploaded yet the patch that binds qcom vops to the driver, but
> we will soon.

Perhaps you could make that patch part of v2 of this series. I see
little point in this series without that patch. Perhaps someone else
still cares about it, but I'm not looking at it anymore.

Thanks,


Paul Bolle

Re: [PATCH v1 3/3] scsi: ufs-qcom: update configuration option of SCSI_UFS_QCOM component

[ygardi]

> On Thu, 2015-05-21 at 10:09 +0000, ygardi@codeaurora.org wrote:
>> > On Wed, 2015-05-20 at 10:22 +0200, Paul Bolle wrote:
>> > Am I missing something obvious here? Because ufs-qcom currently looks
>> > pointless to me, and I actually see little reason to even have it in
>> the
>> > mainline tree.
>> >
>>
>> we haven't uploaded yet the patch that binds qcom vops to the driver,
>> but
>> we will soon.
>
> Perhaps you could make that patch part of v2 of this series. I see
> little point in this series without that patch. Perhaps someone else
> still cares about it, but I'm not looking at it anymore.
>

fair enough. i will upload a V2 series soon. thanks for your inputs.

> Thanks,
>
>
> Paul Bolle
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>