* [PATCH v1 1/1] phy: tusb1210: use bitmasks to set VENDOR_SPECIFIC2
@ 2020-07-02 1:06 Liam Beguin
2020-07-13 5:08 ` Vinod Koul
0 siblings, 1 reply; 3+ messages in thread
From: Liam Beguin @ 2020-07-02 1:06 UTC (permalink / raw)
Cc: liambeguin, kishon, vkoul, 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>
---
drivers/phy/ti/phy-tusb1210.c | 29 ++++++++++++++++-------------
1 file changed, 16 insertions(+), 13 deletions(-)
diff --git a/drivers/phy/ti/phy-tusb1210.c b/drivers/phy/ti/phy-tusb1210.c
index d8d0cc11d187..a957655ebd36 100644
--- a/drivers/phy/ti/phy-tusb1210.c
+++ b/drivers/phy/ti/phy-tusb1210.c
@@ -13,9 +13,9 @@
#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 0x0f
+#define TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_MASK 0x30
+#define TUSB1210_VENDOR_SPECIFIC2_DP_MASK BIT(6)
struct tusb1210 {
struct ulpi *ulpi;
@@ -118,22 +118,25 @@ 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))
+ reg = (reg & ~TUSB1210_VENDOR_SPECIFIC2_IHSTX_MASK) |
+ (val & 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))
+ reg = (reg & ~TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_MASK) |
+ (val & 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))
+ reg = (reg & ~TUSB1210_VENDOR_SPECIFIC2_DP_MASK) |
+ (val & 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: cd77006e01b3198c75fb7819b3d0ff89709539bb
--
2.27.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v1 1/1] phy: tusb1210: use bitmasks to set VENDOR_SPECIFIC2
2020-07-02 1:06 [PATCH v1 1/1] phy: tusb1210: use bitmasks to set VENDOR_SPECIFIC2 Liam Beguin
@ 2020-07-13 5:08 ` Vinod Koul
2020-07-14 16:28 ` Liam Beguin
0 siblings, 1 reply; 3+ messages in thread
From: Vinod Koul @ 2020-07-13 5:08 UTC (permalink / raw)
To: Liam Beguin; +Cc: kishon, linux-kernel
On 01-07-20, 21:06, 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>
> ---
> drivers/phy/ti/phy-tusb1210.c | 29 ++++++++++++++++-------------
> 1 file changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/phy/ti/phy-tusb1210.c b/drivers/phy/ti/phy-tusb1210.c
> index d8d0cc11d187..a957655ebd36 100644
> --- a/drivers/phy/ti/phy-tusb1210.c
> +++ b/drivers/phy/ti/phy-tusb1210.c
> @@ -13,9 +13,9 @@
> #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 0x0f
> +#define TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_MASK 0x30
Why not be better and use GENMASK(3, 0) and GENMASK(5, 4) for these
> +#define TUSB1210_VENDOR_SPECIFIC2_DP_MASK BIT(6)
>
> struct tusb1210 {
> struct ulpi *ulpi;
> @@ -118,22 +118,25 @@ 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))
> + reg = (reg & ~TUSB1210_VENDOR_SPECIFIC2_IHSTX_MASK) |
> + (val & TUSB1210_VENDOR_SPECIFIC2_IHSTX_MASK);
It would be better to use a helper to do this
>
> /* 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))
> + reg = (reg & ~TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_MASK) |
> + (val & 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))
> + reg = (reg & ~TUSB1210_VENDOR_SPECIFIC2_DP_MASK) |
> + (val & 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: cd77006e01b3198c75fb7819b3d0ff89709539bb
fatal: bad object cd77006e01b3198c75fb7819b3d0ff89709539bb in
linux-phy.git
--
~Vinod
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v1 1/1] phy: tusb1210: use bitmasks to set VENDOR_SPECIFIC2
2020-07-13 5:08 ` Vinod Koul
@ 2020-07-14 16:28 ` Liam Beguin
0 siblings, 0 replies; 3+ messages in thread
From: Liam Beguin @ 2020-07-14 16:28 UTC (permalink / raw)
To: Vinod Koul; +Cc: kishon, linux-kernel
Hi Vinod,
On Mon Jul 13, 2020 at 10:38 AM Vinod Koul wrote:
> On 01-07-20, 21:06, 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>
> > ---
> > drivers/phy/ti/phy-tusb1210.c | 29 ++++++++++++++++-------------
> > 1 file changed, 16 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/phy/ti/phy-tusb1210.c b/drivers/phy/ti/phy-tusb1210.c
> > index d8d0cc11d187..a957655ebd36 100644
> > --- a/drivers/phy/ti/phy-tusb1210.c
> > +++ b/drivers/phy/ti/phy-tusb1210.c
> > @@ -13,9 +13,9 @@
> > #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 0x0f
> > +#define TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_MASK 0x30
>
> Why not be better and use GENMASK(3, 0) and GENMASK(5, 4) for these
>
Will update, I didn't know about GENMASK.
> > +#define TUSB1210_VENDOR_SPECIFIC2_DP_MASK BIT(6)
> >
> > struct tusb1210 {
> > struct ulpi *ulpi;
> > @@ -118,22 +118,25 @@ 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))
> > + reg = (reg & ~TUSB1210_VENDOR_SPECIFIC2_IHSTX_MASK) |
> > + (val & TUSB1210_VENDOR_SPECIFIC2_IHSTX_MASK);
>
> It would be better to use a helper to do this
>
I found set_mask_bits(), will use that instead.
> >
> > /* 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))
> > + reg = (reg & ~TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_MASK) |
> > + (val & 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))
> > + reg = (reg & ~TUSB1210_VENDOR_SPECIFIC2_DP_MASK) |
> > + (val & 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: cd77006e01b3198c75fb7819b3d0ff89709539bb
>
> fatal: bad object cd77006e01b3198c75fb7819b3d0ff89709539bb in
> linux-phy.git
>
Sorry about that, I'll rebase on the latest linux-phy.git
Thanks for your time,
Liam
> --
> ~Vinod
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-07-14 16:28 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-02 1:06 [PATCH v1 1/1] phy: tusb1210: use bitmasks to set VENDOR_SPECIFIC2 Liam Beguin
2020-07-13 5:08 ` Vinod Koul
2020-07-14 16:28 ` Liam Beguin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.