linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 1/1] phy: tusb1210: use bitmasks to set VENDOR_SPECIFIC2
@ 2020-08-22 20:53 Liam Beguin
  2020-08-31 11:08 ` Vinod Koul
  0 siblings, 1 reply; 4+ messages in thread
From: Liam Beguin @ 2020-08-22 20:53 UTC (permalink / raw)
  To: liambeguin, geert, kishon, vkoul; +Cc: linux-kernel

From: Liam Beguin <lvb@xiphos.com>

Start by reading the content of the VENDOR_SPECIFIC2 register and update
each bit field based on device properties when defined.

The use of bit masks prevents fields from overriding each other and
enables users to clear bits which are set by default, like datapolarity
in this instance.

Signed-off-by: Liam Beguin <lvb@xiphos.com>
---
Changes since v1:
- use set_mask_bits

Changes since v2:
- fix missing bit shift dropped in v2
- rebase on 5.9-rc1

Changes since v3:
- switch to u8p_replace_bits() since there's little reason to protect
  against concurrent access

 drivers/phy/ti/phy-tusb1210.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/phy/ti/phy-tusb1210.c b/drivers/phy/ti/phy-tusb1210.c
index d8d0cc11d187..a63213f5972a 100644
--- a/drivers/phy/ti/phy-tusb1210.c
+++ b/drivers/phy/ti/phy-tusb1210.c
@@ -7,15 +7,16 @@
  * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
  */
 #include <linux/module.h>
+#include <linux/bitfield.h>
 #include <linux/ulpi/driver.h>
 #include <linux/ulpi/regs.h>
 #include <linux/gpio/consumer.h>
 #include <linux/phy/ulpi_phy.h>
 
 #define TUSB1210_VENDOR_SPECIFIC2		0x80
-#define TUSB1210_VENDOR_SPECIFIC2_IHSTX_SHIFT	0
-#define TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_SHIFT	4
-#define TUSB1210_VENDOR_SPECIFIC2_DP_SHIFT	6
+#define TUSB1210_VENDOR_SPECIFIC2_IHSTX_MASK	GENMASK(3, 0)
+#define TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_MASK	GENMASK(5, 4)
+#define TUSB1210_VENDOR_SPECIFIC2_DP_MASK	BIT(6)
 
 struct tusb1210 {
 	struct ulpi *ulpi;
@@ -118,22 +119,22 @@ static int tusb1210_probe(struct ulpi *ulpi)
 	 * diagram optimization and DP/DM swap.
 	 */
 
+	reg = ulpi_read(ulpi, TUSB1210_VENDOR_SPECIFIC2);
+
 	/* High speed output drive strength configuration */
-	device_property_read_u8(&ulpi->dev, "ihstx", &val);
-	reg = val << TUSB1210_VENDOR_SPECIFIC2_IHSTX_SHIFT;
+	if (!device_property_read_u8(&ulpi->dev, "ihstx", &val))
+		u8p_replace_bits(&reg, val, (u8)TUSB1210_VENDOR_SPECIFIC2_IHSTX_MASK);
 
 	/* High speed output impedance configuration */
-	device_property_read_u8(&ulpi->dev, "zhsdrv", &val);
-	reg |= val << TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_SHIFT;
+	if (!device_property_read_u8(&ulpi->dev, "zhsdrv", &val))
+		u8p_replace_bits(&reg, val, (u8)TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_MASK);
 
 	/* DP/DM swap control */
-	device_property_read_u8(&ulpi->dev, "datapolarity", &val);
-	reg |= val << TUSB1210_VENDOR_SPECIFIC2_DP_SHIFT;
+	if (!device_property_read_u8(&ulpi->dev, "datapolarity", &val))
+		u8p_replace_bits(&reg, val, (u8)TUSB1210_VENDOR_SPECIFIC2_DP_MASK);
 
-	if (reg) {
-		ulpi_write(ulpi, TUSB1210_VENDOR_SPECIFIC2, reg);
-		tusb->vendor_specific2 = reg;
-	}
+	ulpi_write(ulpi, TUSB1210_VENDOR_SPECIFIC2, reg);
+	tusb->vendor_specific2 = reg;
 
 	tusb->phy = ulpi_phy_create(ulpi, &phy_ops);
 	if (IS_ERR(tusb->phy))

base-commit: 9123e3a74ec7b934a4a099e98af6a61c2f80bbf5
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH v4 1/1] phy: tusb1210: use bitmasks to set VENDOR_SPECIFIC2
  2020-08-22 20:53 [PATCH v4 1/1] phy: tusb1210: use bitmasks to set VENDOR_SPECIFIC2 Liam Beguin
@ 2020-08-31 11:08 ` Vinod Koul
  2020-08-31 16:10   ` Liam Beguin
  2020-09-01 14:11   ` Liam Beguin
  0 siblings, 2 replies; 4+ messages in thread
From: Vinod Koul @ 2020-08-31 11:08 UTC (permalink / raw)
  To: Liam Beguin; +Cc: geert, kishon, linux-kernel

Hi Liam,

On 22-08-20, 16:53, Liam Beguin wrote:
> From: Liam Beguin <lvb@xiphos.com>
> 
> Start by reading the content of the VENDOR_SPECIFIC2 register and update
> each bit field based on device properties when defined.
> 
> The use of bit masks prevents fields from overriding each other and
> enables users to clear bits which are set by default, like datapolarity
> in this instance.
> 
> Signed-off-by: Liam Beguin <lvb@xiphos.com>
> ---
> Changes since v1:
> - use set_mask_bits
> 
> Changes since v2:
> - fix missing bit shift dropped in v2
> - rebase on 5.9-rc1
> 
> Changes since v3:
> - switch to u8p_replace_bits() since there's little reason to protect
>   against concurrent access
> 
>  drivers/phy/ti/phy-tusb1210.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/phy/ti/phy-tusb1210.c b/drivers/phy/ti/phy-tusb1210.c
> index d8d0cc11d187..a63213f5972a 100644
> --- a/drivers/phy/ti/phy-tusb1210.c
> +++ b/drivers/phy/ti/phy-tusb1210.c
> @@ -7,15 +7,16 @@
>   * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>   */
>  #include <linux/module.h>
> +#include <linux/bitfield.h>
>  #include <linux/ulpi/driver.h>
>  #include <linux/ulpi/regs.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/phy/ulpi_phy.h>
>  
>  #define TUSB1210_VENDOR_SPECIFIC2		0x80
> -#define TUSB1210_VENDOR_SPECIFIC2_IHSTX_SHIFT	0
> -#define TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_SHIFT	4
> -#define TUSB1210_VENDOR_SPECIFIC2_DP_SHIFT	6
> +#define TUSB1210_VENDOR_SPECIFIC2_IHSTX_MASK	GENMASK(3, 0)
> +#define TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_MASK	GENMASK(5, 4)
> +#define TUSB1210_VENDOR_SPECIFIC2_DP_MASK	BIT(6)
>  
>  struct tusb1210 {
>  	struct ulpi *ulpi;
> @@ -118,22 +119,22 @@ static int tusb1210_probe(struct ulpi *ulpi)
>  	 * diagram optimization and DP/DM swap.
>  	 */
>  
> +	reg = ulpi_read(ulpi, TUSB1210_VENDOR_SPECIFIC2);
> +
>  	/* High speed output drive strength configuration */
> -	device_property_read_u8(&ulpi->dev, "ihstx", &val);
> -	reg = val << TUSB1210_VENDOR_SPECIFIC2_IHSTX_SHIFT;
> +	if (!device_property_read_u8(&ulpi->dev, "ihstx", &val))
> +		u8p_replace_bits(&reg, val, (u8)TUSB1210_VENDOR_SPECIFIC2_IHSTX_MASK);

Any reason for using u8p_replace_bits and not u8_replace_bits? 

Also please drop the u8 casts above, they seem unnecessary here

>  
>  	/* High speed output impedance configuration */
> -	device_property_read_u8(&ulpi->dev, "zhsdrv", &val);
> -	reg |= val << TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_SHIFT;
> +	if (!device_property_read_u8(&ulpi->dev, "zhsdrv", &val))
> +		u8p_replace_bits(&reg, val, (u8)TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_MASK);
>  
>  	/* DP/DM swap control */
> -	device_property_read_u8(&ulpi->dev, "datapolarity", &val);
> -	reg |= val << TUSB1210_VENDOR_SPECIFIC2_DP_SHIFT;
> +	if (!device_property_read_u8(&ulpi->dev, "datapolarity", &val))
> +		u8p_replace_bits(&reg, val, (u8)TUSB1210_VENDOR_SPECIFIC2_DP_MASK);
>  
> -	if (reg) {
> -		ulpi_write(ulpi, TUSB1210_VENDOR_SPECIFIC2, reg);
> -		tusb->vendor_specific2 = reg;
> -	}
> +	ulpi_write(ulpi, TUSB1210_VENDOR_SPECIFIC2, reg);
> +	tusb->vendor_specific2 = reg;
>  
>  	tusb->phy = ulpi_phy_create(ulpi, &phy_ops);
>  	if (IS_ERR(tusb->phy))
> 
> base-commit: 9123e3a74ec7b934a4a099e98af6a61c2f80bbf5
> -- 
> 2.27.0

-- 
~Vinod

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH v4 1/1] phy: tusb1210: use bitmasks to set VENDOR_SPECIFIC2
  2020-08-31 11:08 ` Vinod Koul
@ 2020-08-31 16:10   ` Liam Beguin
  2020-09-01 14:11   ` Liam Beguin
  1 sibling, 0 replies; 4+ messages in thread
From: Liam Beguin @ 2020-08-31 16:10 UTC (permalink / raw)
  To: Vinod Koul; +Cc: geert, kishon, linux-kernel

Hi Vinod,

On Mon Aug 31, 2020 at 7:08 AM EDT, Vinod Koul wrote:
> Hi Liam,
>
> On 22-08-20, 16:53, Liam Beguin wrote:
> > From: Liam Beguin <lvb@xiphos.com>
> > 
> > Start by reading the content of the VENDOR_SPECIFIC2 register and update
> > each bit field based on device properties when defined.
> > 
> > The use of bit masks prevents fields from overriding each other and
> > enables users to clear bits which are set by default, like datapolarity
> > in this instance.
> > 
> > Signed-off-by: Liam Beguin <lvb@xiphos.com>
> > ---
> > Changes since v1:
> > - use set_mask_bits
> > 
> > Changes since v2:
> > - fix missing bit shift dropped in v2
> > - rebase on 5.9-rc1
> > 
> > Changes since v3:
> > - switch to u8p_replace_bits() since there's little reason to protect
> >   against concurrent access
> > 
> >  drivers/phy/ti/phy-tusb1210.c | 27 ++++++++++++++-------------
> >  1 file changed, 14 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/phy/ti/phy-tusb1210.c b/drivers/phy/ti/phy-tusb1210.c
> > index d8d0cc11d187..a63213f5972a 100644
> > --- a/drivers/phy/ti/phy-tusb1210.c
> > +++ b/drivers/phy/ti/phy-tusb1210.c
> > @@ -7,15 +7,16 @@
> >   * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >   */
> >  #include <linux/module.h>
> > +#include <linux/bitfield.h>
> >  #include <linux/ulpi/driver.h>
> >  #include <linux/ulpi/regs.h>
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/phy/ulpi_phy.h>
> >  
> >  #define TUSB1210_VENDOR_SPECIFIC2		0x80
> > -#define TUSB1210_VENDOR_SPECIFIC2_IHSTX_SHIFT	0
> > -#define TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_SHIFT	4
> > -#define TUSB1210_VENDOR_SPECIFIC2_DP_SHIFT	6
> > +#define TUSB1210_VENDOR_SPECIFIC2_IHSTX_MASK	GENMASK(3, 0)
> > +#define TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_MASK	GENMASK(5, 4)
> > +#define TUSB1210_VENDOR_SPECIFIC2_DP_MASK	BIT(6)
> >  
> >  struct tusb1210 {
> >  	struct ulpi *ulpi;
> > @@ -118,22 +119,22 @@ static int tusb1210_probe(struct ulpi *ulpi)
> >  	 * diagram optimization and DP/DM swap.
> >  	 */
> >  
> > +	reg = ulpi_read(ulpi, TUSB1210_VENDOR_SPECIFIC2);
> > +
> >  	/* High speed output drive strength configuration */
> > -	device_property_read_u8(&ulpi->dev, "ihstx", &val);
> > -	reg = val << TUSB1210_VENDOR_SPECIFIC2_IHSTX_SHIFT;
> > +	if (!device_property_read_u8(&ulpi->dev, "ihstx", &val))
> > +		u8p_replace_bits(&reg, val, (u8)TUSB1210_VENDOR_SPECIFIC2_IHSTX_MASK);
>
> Any reason for using u8p_replace_bits and not u8_replace_bits?
>

u8p_replace_bits seems like a more concise notation.

> Also please drop the u8 casts above, they seem unnecessary here
>

I added the casts to get rid of compilation warnings.

> >  
> >  	/* High speed output impedance configuration */
> > -	device_property_read_u8(&ulpi->dev, "zhsdrv", &val);
> > -	reg |= val << TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_SHIFT;
> > +	if (!device_property_read_u8(&ulpi->dev, "zhsdrv", &val))
> > +		u8p_replace_bits(&reg, val, (u8)TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_MASK);
> >  
> >  	/* DP/DM swap control */
> > -	device_property_read_u8(&ulpi->dev, "datapolarity", &val);
> > -	reg |= val << TUSB1210_VENDOR_SPECIFIC2_DP_SHIFT;
> > +	if (!device_property_read_u8(&ulpi->dev, "datapolarity", &val))
> > +		u8p_replace_bits(&reg, val, (u8)TUSB1210_VENDOR_SPECIFIC2_DP_MASK);
> >  
> > -	if (reg) {
> > -		ulpi_write(ulpi, TUSB1210_VENDOR_SPECIFIC2, reg);
> > -		tusb->vendor_specific2 = reg;
> > -	}
> > +	ulpi_write(ulpi, TUSB1210_VENDOR_SPECIFIC2, reg);
> > +	tusb->vendor_specific2 = reg;
> >  
> >  	tusb->phy = ulpi_phy_create(ulpi, &phy_ops);
> >  	if (IS_ERR(tusb->phy))
> > 
> > base-commit: 9123e3a74ec7b934a4a099e98af6a61c2f80bbf5
> > -- 
> > 2.27.0
>
> --
> ~Vinod

Thanks,
Liam

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH v4 1/1] phy: tusb1210: use bitmasks to set VENDOR_SPECIFIC2
  2020-08-31 11:08 ` Vinod Koul
  2020-08-31 16:10   ` Liam Beguin
@ 2020-09-01 14:11   ` Liam Beguin
  1 sibling, 0 replies; 4+ messages in thread
From: Liam Beguin @ 2020-09-01 14:11 UTC (permalink / raw)
  To: Vinod Koul; +Cc: geert, kishon, linux-kernel

Hi Vinod,

On Mon Aug 31, 2020 at 7:08 AM EDT, Vinod Koul wrote:
> Hi Liam,
>
> On 22-08-20, 16:53, Liam Beguin wrote:
> > From: Liam Beguin <lvb@xiphos.com>
> > 
> > Start by reading the content of the VENDOR_SPECIFIC2 register and update
> > each bit field based on device properties when defined.
> > 
> > The use of bit masks prevents fields from overriding each other and
> > enables users to clear bits which are set by default, like datapolarity
> > in this instance.
> > 
> > Signed-off-by: Liam Beguin <lvb@xiphos.com>
> > ---
> > Changes since v1:
> > - use set_mask_bits
> > 
> > Changes since v2:
> > - fix missing bit shift dropped in v2
> > - rebase on 5.9-rc1
> > 
> > Changes since v3:
> > - switch to u8p_replace_bits() since there's little reason to protect
> >   against concurrent access
> > 
> >  drivers/phy/ti/phy-tusb1210.c | 27 ++++++++++++++-------------
> >  1 file changed, 14 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/phy/ti/phy-tusb1210.c b/drivers/phy/ti/phy-tusb1210.c
> > index d8d0cc11d187..a63213f5972a 100644
> > --- a/drivers/phy/ti/phy-tusb1210.c
> > +++ b/drivers/phy/ti/phy-tusb1210.c
> > @@ -7,15 +7,16 @@
> >   * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >   */
> >  #include <linux/module.h>
> > +#include <linux/bitfield.h>
> >  #include <linux/ulpi/driver.h>
> >  #include <linux/ulpi/regs.h>
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/phy/ulpi_phy.h>
> >  
> >  #define TUSB1210_VENDOR_SPECIFIC2		0x80
> > -#define TUSB1210_VENDOR_SPECIFIC2_IHSTX_SHIFT	0
> > -#define TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_SHIFT	4
> > -#define TUSB1210_VENDOR_SPECIFIC2_DP_SHIFT	6
> > +#define TUSB1210_VENDOR_SPECIFIC2_IHSTX_MASK	GENMASK(3, 0)
> > +#define TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_MASK	GENMASK(5, 4)
> > +#define TUSB1210_VENDOR_SPECIFIC2_DP_MASK	BIT(6)
> >  
> >  struct tusb1210 {
> >  	struct ulpi *ulpi;
> > @@ -118,22 +119,22 @@ static int tusb1210_probe(struct ulpi *ulpi)
> >  	 * diagram optimization and DP/DM swap.
> >  	 */
> >  
> > +	reg = ulpi_read(ulpi, TUSB1210_VENDOR_SPECIFIC2);
> > +
> >  	/* High speed output drive strength configuration */
> > -	device_property_read_u8(&ulpi->dev, "ihstx", &val);
> > -	reg = val << TUSB1210_VENDOR_SPECIFIC2_IHSTX_SHIFT;
> > +	if (!device_property_read_u8(&ulpi->dev, "ihstx", &val))
> > +		u8p_replace_bits(&reg, val, (u8)TUSB1210_VENDOR_SPECIFIC2_IHSTX_MASK);
>
> Any reason for using u8p_replace_bits and not u8_replace_bits?
>

u8p_replace_bits seemed like a more concise notation.

> Also please drop the u8 casts above, they seem unnecessary here
>

I added the type casts to get rid of compilation warnings.

> >  
> >  	/* High speed output impedance configuration */
> > -	device_property_read_u8(&ulpi->dev, "zhsdrv", &val);
> > -	reg |= val << TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_SHIFT;
> > +	if (!device_property_read_u8(&ulpi->dev, "zhsdrv", &val))
> > +		u8p_replace_bits(&reg, val, (u8)TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_MASK);
> >  
> >  	/* DP/DM swap control */
> > -	device_property_read_u8(&ulpi->dev, "datapolarity", &val);
> > -	reg |= val << TUSB1210_VENDOR_SPECIFIC2_DP_SHIFT;
> > +	if (!device_property_read_u8(&ulpi->dev, "datapolarity", &val))
> > +		u8p_replace_bits(&reg, val, (u8)TUSB1210_VENDOR_SPECIFIC2_DP_MASK);
> >  
> > -	if (reg) {
> > -		ulpi_write(ulpi, TUSB1210_VENDOR_SPECIFIC2, reg);
> > -		tusb->vendor_specific2 = reg;
> > -	}
> > +	ulpi_write(ulpi, TUSB1210_VENDOR_SPECIFIC2, reg);
> > +	tusb->vendor_specific2 = reg;
> >  
> >  	tusb->phy = ulpi_phy_create(ulpi, &phy_ops);
> >  	if (IS_ERR(tusb->phy))
> > 
> > base-commit: 9123e3a74ec7b934a4a099e98af6a61c2f80bbf5
> > -- 
> > 2.27.0
>
> --
> ~Vinod

Thanks,
Liam

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-09-01 14:48 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-22 20:53 [PATCH v4 1/1] phy: tusb1210: use bitmasks to set VENDOR_SPECIFIC2 Liam Beguin
2020-08-31 11:08 ` Vinod Koul
2020-08-31 16:10   ` Liam Beguin
2020-09-01 14:11   ` Liam Beguin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).