Messages in this thread | ![/](/images/icornerl.gif) | | Date | Mon, 31 Aug 2020 16:38:52 +0530 | From | Vinod Koul <> | Subject | Re: [PATCH v4 1/1] phy: tusb1210: use bitmasks to set VENDOR_SPECIFIC2 |
| |
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(®, 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(®, 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(®, 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
| ![\](/images/icornerr.gif) |