Message ID | 20211218082802.5256-3-chunfeng.yun@mediatek.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v2,1/5] dt-bindings: phy: mediatek: tphy: support software efuse load | expand |
Il 18/12/21 09:28, Chunfeng Yun ha scritto: > Add three helpers mtk_phy_clear/set/update_bits() for registers operation > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > --- > v2: new patch, add register access helpers, > Add updatel() macro suggested by Vinod, here add more ones instead. > --- > drivers/phy/mediatek/phy-mtk-io.h | 38 +++++++++++++++++++++++++++++++ > 1 file changed, 38 insertions(+) > create mode 100644 drivers/phy/mediatek/phy-mtk-io.h > > diff --git a/drivers/phy/mediatek/phy-mtk-io.h b/drivers/phy/mediatek/phy-mtk-io.h > new file mode 100644 > index 000000000000..500fcdab165d > --- /dev/null > +++ b/drivers/phy/mediatek/phy-mtk-io.h > @@ -0,0 +1,38 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (C) 2021 MediaTek Inc. > + * > + * Author: Chunfeng Yun <chunfeng.yun@mediatek.com> > + */ > + > +#ifndef __PHY_MTK_H__ > +#define __PHY_MTK_H__ > + > +#include <linux/io.h> > + > +static inline void mtk_phy_clear_bits(void __iomem *reg, u32 bits) > +{ > + u32 tmp = readl(reg); > + > + tmp &= ~bits; > + writel(tmp, reg); > +} > + > +static inline void mtk_phy_set_bits(void __iomem *reg, u32 bits) > +{ > + u32 tmp = readl(reg); > + > + tmp |= bits; > + writel(tmp, reg); > +} > + > +static inline void mtk_phy_update_bits(void __iomem *reg, u32 mask, u32 val) > +{ > + u32 tmp = readl(reg); > + > + tmp &= ~mask; > + tmp |= val & mask; > + writel(tmp, reg); > +} > + > +#endif > These helpers are almost exactly duplicating what regmap_update_bits() is doing. I appreciate the effort to stop open-coding the same sequences over and over by adding such helper functions, but I think that the proper way of doing what you are proposing is not to add custom functions but rather reuse what the Linux APIs give you. What about doing a conversion to use regmap on this driver?
On Fri, 2021-12-24 at 11:10 +0100, AngeloGioacchino Del Regno wrote: > Il 18/12/21 09:28, Chunfeng Yun ha scritto: > > Add three helpers mtk_phy_clear/set/update_bits() for registers > > operation > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > > --- > > v2: new patch, add register access helpers, > > Add updatel() macro suggested by Vinod, here add more ones > > instead. > > --- > > drivers/phy/mediatek/phy-mtk-io.h | 38 > > +++++++++++++++++++++++++++++++ > > 1 file changed, 38 insertions(+) > > create mode 100644 drivers/phy/mediatek/phy-mtk-io.h > > > > diff --git a/drivers/phy/mediatek/phy-mtk-io.h > > b/drivers/phy/mediatek/phy-mtk-io.h > > new file mode 100644 > > index 000000000000..500fcdab165d > > --- /dev/null > > +++ b/drivers/phy/mediatek/phy-mtk-io.h > > @@ -0,0 +1,38 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Copyright (C) 2021 MediaTek Inc. > > + * > > + * Author: Chunfeng Yun <chunfeng.yun@mediatek.com> > > + */ > > + > > +#ifndef __PHY_MTK_H__ > > +#define __PHY_MTK_H__ > > + > > +#include <linux/io.h> > > + > > +static inline void mtk_phy_clear_bits(void __iomem *reg, u32 bits) > > +{ > > + u32 tmp = readl(reg); > > + > > + tmp &= ~bits; > > + writel(tmp, reg); > > +} > > + > > +static inline void mtk_phy_set_bits(void __iomem *reg, u32 bits) > > +{ > > + u32 tmp = readl(reg); > > + > > + tmp |= bits; > > + writel(tmp, reg); > > +} > > + > > +static inline void mtk_phy_update_bits(void __iomem *reg, u32 > > mask, u32 val) > > +{ > > + u32 tmp = readl(reg); > > + > > + tmp &= ~mask; > > + tmp |= val & mask; > > + writel(tmp, reg); > > +} > > + > > +#endif > > > > These helpers are almost exactly duplicating what > regmap_update_bits() is doing. > I appreciate the effort to stop open-coding the same sequences over > and over by > adding such helper functions, I agree with you. > but I think that the proper way of doing what you > are proposing is not to add custom functions but rather reuse what > the Linux APIs > give you. I also like to use common APIs ASAP, but not found suitable ones. This may be a problem, I found that some similar custom helps already added under phy fold. > > What about doing a conversion to use regmap on this driver? No, we don't use regmap here, these registers are monopolized by t-phy, it's not syscon.
Il 30/12/21 03:06, Chunfeng Yun ha scritto: > On Fri, 2021-12-24 at 11:10 +0100, AngeloGioacchino Del Regno wrote: >> Il 18/12/21 09:28, Chunfeng Yun ha scritto: >>> Add three helpers mtk_phy_clear/set/update_bits() for registers >>> operation >>> >>> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> >>> --- >>> v2: new patch, add register access helpers, >>> Add updatel() macro suggested by Vinod, here add more ones >>> instead. >>> --- >>> drivers/phy/mediatek/phy-mtk-io.h | 38 >>> +++++++++++++++++++++++++++++++ >>> 1 file changed, 38 insertions(+) >>> create mode 100644 drivers/phy/mediatek/phy-mtk-io.h >>> >>> diff --git a/drivers/phy/mediatek/phy-mtk-io.h >>> b/drivers/phy/mediatek/phy-mtk-io.h >>> new file mode 100644 >>> index 000000000000..500fcdab165d >>> --- /dev/null >>> +++ b/drivers/phy/mediatek/phy-mtk-io.h >>> @@ -0,0 +1,38 @@ >>> +/* SPDX-License-Identifier: GPL-2.0 */ >>> +/* >>> + * Copyright (C) 2021 MediaTek Inc. >>> + * >>> + * Author: Chunfeng Yun <chunfeng.yun@mediatek.com> >>> + */ >>> + >>> +#ifndef __PHY_MTK_H__ >>> +#define __PHY_MTK_H__ >>> + >>> +#include <linux/io.h> >>> + >>> +static inline void mtk_phy_clear_bits(void __iomem *reg, u32 bits) >>> +{ >>> + u32 tmp = readl(reg); >>> + >>> + tmp &= ~bits; >>> + writel(tmp, reg); >>> +} >>> + >>> +static inline void mtk_phy_set_bits(void __iomem *reg, u32 bits) >>> +{ >>> + u32 tmp = readl(reg); >>> + >>> + tmp |= bits; >>> + writel(tmp, reg); >>> +} >>> + >>> +static inline void mtk_phy_update_bits(void __iomem *reg, u32 >>> mask, u32 val) >>> +{ >>> + u32 tmp = readl(reg); >>> + >>> + tmp &= ~mask; >>> + tmp |= val & mask; >>> + writel(tmp, reg); >>> +} >>> + >>> +#endif >>> >> >> These helpers are almost exactly duplicating what >> regmap_update_bits() is doing. >> I appreciate the effort to stop open-coding the same sequences over >> and over by >> adding such helper functions, > I agree with you. >> but I think that the proper way of doing what you >> are proposing is not to add custom functions but rather reuse what >> the Linux APIs >> give you. > I also like to use common APIs ASAP, but not found suitable ones. > This may be a problem, I found that some similar custom helps already > added under phy fold. > >> >> What about doing a conversion to use regmap on this driver? > No, we don't use regmap here, these registers are monopolized by t-phy, > it's not syscon. > > Hello, The regmap API allows this kind of usage, registers don't necessarily have to be part of a syscon. Regards, - Angelo
On Mon, 2022-01-03 at 09:59 +0100, AngeloGioacchino Del Regno wrote: > Il 30/12/21 03:06, Chunfeng Yun ha scritto: > > On Fri, 2021-12-24 at 11:10 +0100, AngeloGioacchino Del Regno > > wrote: > > > Il 18/12/21 09:28, Chunfeng Yun ha scritto: > > > > Add three helpers mtk_phy_clear/set/update_bits() for registers > > > > operation > > > > > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > > > > --- > > > > v2: new patch, add register access helpers, > > > > Add updatel() macro suggested by Vinod, here add more > > > > ones > > > > instead. > > > > --- > > > > drivers/phy/mediatek/phy-mtk-io.h | 38 > > > > +++++++++++++++++++++++++++++++ > > > > 1 file changed, 38 insertions(+) > > > > create mode 100644 drivers/phy/mediatek/phy-mtk-io.h > > > > > > > > diff --git a/drivers/phy/mediatek/phy-mtk-io.h > > > > b/drivers/phy/mediatek/phy-mtk-io.h > > > > new file mode 100644 > > > > index 000000000000..500fcdab165d > > > > --- /dev/null > > > > +++ b/drivers/phy/mediatek/phy-mtk-io.h > > > > @@ -0,0 +1,38 @@ > > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > > +/* > > > > + * Copyright (C) 2021 MediaTek Inc. > > > > + * > > > > + * Author: Chunfeng Yun <chunfeng.yun@mediatek.com> > > > > + */ > > > > + > > > > +#ifndef __PHY_MTK_H__ > > > > +#define __PHY_MTK_H__ > > > > + > > > > +#include <linux/io.h> > > > > + > > > > +static inline void mtk_phy_clear_bits(void __iomem *reg, u32 > > > > bits) > > > > +{ > > > > + u32 tmp = readl(reg); > > > > + > > > > + tmp &= ~bits; > > > > + writel(tmp, reg); > > > > +} > > > > + > > > > +static inline void mtk_phy_set_bits(void __iomem *reg, u32 > > > > bits) > > > > +{ > > > > + u32 tmp = readl(reg); > > > > + > > > > + tmp |= bits; > > > > + writel(tmp, reg); > > > > +} > > > > + > > > > +static inline void mtk_phy_update_bits(void __iomem *reg, u32 > > > > mask, u32 val) > > > > +{ > > > > + u32 tmp = readl(reg); > > > > + > > > > + tmp &= ~mask; > > > > + tmp |= val & mask; > > > > + writel(tmp, reg); > > > > +} > > > > + > > > > +#endif > > > > > > > > > > These helpers are almost exactly duplicating what > > > regmap_update_bits() is doing. > > > I appreciate the effort to stop open-coding the same sequences > > > over > > > and over by > > > adding such helper functions, > > > > I agree with you. > > > but I think that the proper way of doing what you > > > are proposing is not to add custom functions but rather reuse > > > what > > > the Linux APIs > > > give you. > > > > I also like to use common APIs ASAP, but not found suitable ones. > > This may be a problem, I found that some similar custom helps > > already > > added under phy fold. > > > > > > > > What about doing a conversion to use regmap on this driver? > > > > No, we don't use regmap here, these registers are monopolized by t- > > phy, > > it's not syscon. > > > > > > Hello, > > The regmap API allows this kind of usage, registers don't necessarily > have > to be part of a syscon. I'm sorry, I don't want to make it more complex > > Regards, > - Angelo
diff --git a/drivers/phy/mediatek/phy-mtk-io.h b/drivers/phy/mediatek/phy-mtk-io.h new file mode 100644 index 000000000000..500fcdab165d --- /dev/null +++ b/drivers/phy/mediatek/phy-mtk-io.h @@ -0,0 +1,38 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (C) 2021 MediaTek Inc. + * + * Author: Chunfeng Yun <chunfeng.yun@mediatek.com> + */ + +#ifndef __PHY_MTK_H__ +#define __PHY_MTK_H__ + +#include <linux/io.h> + +static inline void mtk_phy_clear_bits(void __iomem *reg, u32 bits) +{ + u32 tmp = readl(reg); + + tmp &= ~bits; + writel(tmp, reg); +} + +static inline void mtk_phy_set_bits(void __iomem *reg, u32 bits) +{ + u32 tmp = readl(reg); + + tmp |= bits; + writel(tmp, reg); +} + +static inline void mtk_phy_update_bits(void __iomem *reg, u32 mask, u32 val) +{ + u32 tmp = readl(reg); + + tmp &= ~mask; + tmp |= val & mask; + writel(tmp, reg); +} + +#endif
Add three helpers mtk_phy_clear/set/update_bits() for registers operation Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> --- v2: new patch, add register access helpers, Add updatel() macro suggested by Vinod, here add more ones instead. --- drivers/phy/mediatek/phy-mtk-io.h | 38 +++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 drivers/phy/mediatek/phy-mtk-io.h