| Message ID | cover.1603877481.git.matti.vaittinen@fi.rohmeurope.com |
|---|---|
| Headers | show |
| Series | Support ROHM BD9576MUF and BD9573MUF PMICs | expand |
On Wed, 28 Oct 2020, Matti Vaittinen wrote: > Add core support for ROHM BD9576MUF and BD9573MUF PMICs which are > mainly used to power the R-Car series processors. > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > --- > drivers/mfd/Kconfig | 11 +++ > drivers/mfd/Makefile | 1 + > drivers/mfd/rohm-bd9576.c | 130 +++++++++++++++++++++++++++++++ > include/linux/mfd/rohm-bd957x.h | 59 ++++++++++++++ > include/linux/mfd/rohm-generic.h | 2 + > 5 files changed, 203 insertions(+) > create mode 100644 drivers/mfd/rohm-bd9576.c > create mode 100644 include/linux/mfd/rohm-bd957x.h > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index 8b99a13669bf..dcb2b14a570e 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -2010,6 +2010,17 @@ config MFD_ROHM_BD71828 > Also included is a Coulomb counter, a real-time clock (RTC), and > a 32.768 kHz clock gate. > > +config MFD_ROHM_BD957XMUF > + tristate "ROHM BD9576MUF and BD9573MUF Power Management ICs" > + depends on I2C=y > + depends on OF > + select REGMAP_I2C > + select MFD_CORE > + help > + Select this option to get support for the ROHM BD9576MUF and > + BD9573MUF Power Management ICs. BD9576 and BD9573 are primarily > + designed to be used to power R-Car series processors. > + > config MFD_STM32_LPTIMER > tristate "Support for STM32 Low-Power Timer" > depends on (ARCH_STM32 && OF) || COMPILE_TEST > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index 1780019d2474..837f68c9f336 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -261,6 +261,7 @@ obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o > obj-$(CONFIG_MFD_ROHM_BD70528) += rohm-bd70528.o > obj-$(CONFIG_MFD_ROHM_BD71828) += rohm-bd71828.o > obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o > +obj-$(CONFIG_MFD_ROHM_BD957XMUF) += rohm-bd9576.o > obj-$(CONFIG_MFD_STMFX) += stmfx.o > obj-$(CONFIG_MFD_KHADAS_MCU) += khadas-mcu.o > > diff --git a/drivers/mfd/rohm-bd9576.c b/drivers/mfd/rohm-bd9576.c > new file mode 100644 > index 000000000000..a23ded510209 > --- /dev/null > +++ b/drivers/mfd/rohm-bd9576.c > @@ -0,0 +1,130 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +// > +// Copyright (C) 2020 ROHM Semiconductors > +// > +// ROHM BD9576MUF and BD9573MUF PMIC driver > + > +#include <linux/i2c.h> > +#include <linux/interrupt.h> > +#include <linux/ioport.h> > +#include <linux/irq.h> > +#include <linux/mfd/core.h> > +#include <linux/mfd/rohm-bd957x.h> > +#include <linux/mfd/rohm-generic.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/regmap.h> > +#include <linux/types.h> > + > +static struct mfd_cell bd9573_mfd_cells[] = { > + { .name = "bd9573-pmic", }, > + { .name = "bd9576-wdt", }, > +}; > + > + > +static struct mfd_cell bd9576_mfd_cells[] = { > + { .name = "bd9576-pmic", }, > + { .name = "bd9576-wdt", }, > +}; > + > +static const struct regmap_range volatile_ranges[] = { > + { > + .range_min = BD957X_REG_SMRB_ASSERT, > + .range_max = BD957X_REG_SMRB_ASSERT, > + }, > + { The way you space your braces is not consistent. > + .range_min = BD957X_REG_PMIC_INTERNAL_STAT, > + .range_max = BD957X_REG_PMIC_INTERNAL_STAT, > + }, > + { > + .range_min = BD957X_REG_INT_THERM_STAT, > + .range_max = BD957X_REG_INT_THERM_STAT, > + }, > + { > + .range_min = BD957X_REG_INT_OVP_STAT, > + .range_max = BD957X_REG_INT_SYS_STAT, > + }, { > + .range_min = BD957X_REG_INT_MAIN_STAT, > + .range_max = BD957X_REG_INT_MAIN_STAT, > + }, > +}; Please use: regmap_reg_range() > +static const struct regmap_access_table volatile_regs = { > + .yes_ranges = &volatile_ranges[0], > + .n_yes_ranges = ARRAY_SIZE(volatile_ranges), > +}; > + > +static struct regmap_config bd957x_regmap = { > + .reg_bits = 8, > + .val_bits = 8, > + .volatile_table = &volatile_regs, > + .max_register = BD957X_MAX_REGISTER, > + .cache_type = REGCACHE_RBTREE, > +}; > + > +static int bd957x_i2c_probe(struct i2c_client *i2c, > + const struct i2c_device_id *id) > +{ > + int ret; > + struct regmap *regmap; > + struct mfd_cell *mfd; > + int cells; > + unsigned int chip_type; > + > + chip_type = (unsigned int)(uintptr_t) > + of_device_get_match_data(&i2c->dev); Not overly keen on this casting. Why not just leave it as (uintptr_t)? What happens when you don't cast to (uintptr_t) first? > + switch (chip_type) { > + case ROHM_CHIP_TYPE_BD9576: > + mfd = bd9576_mfd_cells; > + cells = ARRAY_SIZE(bd9576_mfd_cells); > + break; > + case ROHM_CHIP_TYPE_BD9573: > + mfd = bd9573_mfd_cells; > + cells = ARRAY_SIZE(bd9573_mfd_cells); > + break; > + default: > + dev_err(&i2c->dev, "Unknown device type"); > + return -EINVAL; > + } > + > + regmap = devm_regmap_init_i2c(i2c, &bd957x_regmap); > + if (IS_ERR(regmap)) { > + dev_err(&i2c->dev, "Failed to initialize Regmap\n"); > + return PTR_ERR(regmap); > + } > + > + ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, mfd, cells, > + NULL, 0, NULL); > + if (ret) > + dev_err(&i2c->dev, "Failed to create subdevices\n"); > + > + return ret; > +} > + > +static const struct of_device_id bd957x_of_match[] = { > + { > + .compatible = "rohm,bd9576", > + .data = (void *)ROHM_CHIP_TYPE_BD9576, > + }, > + { You could put the 2 lines above on a single line. > + .compatible = "rohm,bd9573", > + .data = (void *)ROHM_CHIP_TYPE_BD9573, > + }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, bd957x_of_match); > + > +static struct i2c_driver bd957x_drv = { > + .driver = { > + .name = "rohm-bd957x", > + .of_match_table = bd957x_of_match, > + }, > + .probe = &bd957x_i2c_probe, > +}; > + Remove this line please. > +module_i2c_driver(bd957x_drv); > + > +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>"); > +MODULE_DESCRIPTION("ROHM BD9576MUF and BD9573MUF Power Management IC driver"); > +MODULE_LICENSE("GPL"); > diff --git a/include/linux/mfd/rohm-bd957x.h b/include/linux/mfd/rohm-bd957x.h > new file mode 100644 > index 000000000000..3e7ca6fe5d4f > --- /dev/null > +++ b/include/linux/mfd/rohm-bd957x.h > @@ -0,0 +1,59 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* Copyright (C) 2020 ROHM Semiconductors */ > + > +#ifndef __LINUX_MFD_BD957X_H__ > +#define __LINUX_MFD_BD957X_H__ > + > +enum { > + BD957X_VD50, > + BD957X_VD18, > + BD957X_VDDDR, > + BD957X_VD10, > + BD957X_VOUTL1, > + BD957X_VOUTS1, > +}; > + > +#define BD957X_REG_SMRB_ASSERT 0x15 > +#define BD957X_REG_PMIC_INTERNAL_STAT 0x20 > +#define BD957X_REG_INT_THERM_STAT 0x23 > +#define BD957X_REG_INT_THERM_MASK 0x24 > +#define BD957X_REG_INT_OVP_STAT 0x25 > +#define BD957X_REG_INT_SCP_STAT 0x26 > +#define BD957X_REG_INT_OCP_STAT 0x27 > +#define BD957X_REG_INT_OVD_STAT 0x28 > +#define BD957X_REG_INT_UVD_STAT 0x29 > +#define BD957X_REG_INT_UVP_STAT 0x2a > +#define BD957X_REG_INT_SYS_STAT 0x2b > +#define BD957X_REG_INT_SYS_MASK 0x2c > +#define BD957X_REG_INT_MAIN_STAT 0x30 > +#define BD957X_REG_INT_MAIN_MASK 0x31 > + > +#define BD957X_REG_WDT_CONF 0x16 > + > +#define BD957X_REG_POW_TRIGGER1 0x41 > +#define BD957X_REG_POW_TRIGGER2 0x42 > +#define BD957X_REG_POW_TRIGGER3 0x43 > +#define BD957X_REG_POW_TRIGGER4 0x44 > +#define BD957X_REG_POW_TRIGGERL1 0x45 > +#define BD957X_REG_POW_TRIGGERS1 0x46 > + > +#define BD957X_REGULATOR_EN_MASK 0xff > +#define BD957X_REGULATOR_DIS_VAL 0xff > + > +#define BD957X_VSEL_REG_MASK 0xff > + > +#define BD957X_MASK_VOUT1_TUNE 0x87 > +#define BD957X_MASK_VOUT2_TUNE 0x87 > +#define BD957X_MASK_VOUT3_TUNE 0x1f > +#define BD957X_MASK_VOUT4_TUNE 0x1f > +#define BD957X_MASK_VOUTL1_TUNE 0x87 > + > +#define BD957X_REG_VOUT1_TUNE 0x50 > +#define BD957X_REG_VOUT2_TUNE 0x53 > +#define BD957X_REG_VOUT3_TUNE 0x56 > +#define BD957X_REG_VOUT4_TUNE 0x59 > +#define BD957X_REG_VOUTL1_TUNE 0x5c > + > +#define BD957X_MAX_REGISTER 0x61 > + > +#endif > diff --git a/include/linux/mfd/rohm-generic.h b/include/linux/mfd/rohm-generic.h > index 4283b5b33e04..58b4f1a0f4af 100644 > --- a/include/linux/mfd/rohm-generic.h > +++ b/include/linux/mfd/rohm-generic.h > @@ -12,6 +12,8 @@ enum rohm_chip_type { > ROHM_CHIP_TYPE_BD71847, > ROHM_CHIP_TYPE_BD70528, > ROHM_CHIP_TYPE_BD71828, > + ROHM_CHIP_TYPE_BD9576, > + ROHM_CHIP_TYPE_BD9573, > ROHM_CHIP_TYPE_AMOUNT > }; >
Morning Lee, Thanks for taking a look at this :) I see most of the comments being valid. There's two I would like to clarify though... On Wed, 2020-11-04 at 15:51 +0000, Lee Jones wrote: > On Wed, 28 Oct 2020, Matti Vaittinen wrote: > > > Add core support for ROHM BD9576MUF and BD9573MUF PMICs which are > > mainly used to power the R-Car series processors. > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > --- > > + unsigned int chip_type; > > + > > + chip_type = (unsigned int)(uintptr_t) > > + of_device_get_match_data(&i2c->dev); > > Not overly keen on this casting. > > Why not just leave it as (uintptr_t)? I didn't do so because on x86_64 the address width is probably 64 bits whereas the unsigned int is likely to be 32 bits. So the assignment will crop half of the value. It does not really matter as values are small - but I would be surprized if no compilers/analyzers emitted a warning. I must admit I am not 100% sure though. I sure can change this if you know it better? > What happens when you don't cast to (uintptr_t) first? On some systems at least the gcc will warn: > warning: cast from pointer to integer of different size [-Wpointer- to-int-cast] I am pretty sure I did end up this double casting via trial and error :) + > > +static const struct of_device_id bd957x_of_match[] = { > > + { > > + .compatible = "rohm,bd9576", > > + .data = (void *)ROHM_CHIP_TYPE_BD9576, > > + }, > > + { > > You could put the 2 lines above on a single line. Braces? I put braces on separate lines on purpose. Been doing this after we had this discussion: https://lore.kernel.org/lkml/20180705055226.GJ496@dell/ https://lore.kernel.org/lkml/20180706070559.GW496@dell/ ;) I can change it if you wishfeel it is important - not a point I feel like fighting over ;) > > > + .compatible = "rohm,bd9573", > > + .data = (void *)ROHM_CHIP_TYPE_BD9573, > > + }, > > + { }, > > +}; > > +MODULE_DEVICE_TABLE(of, bd957x_of_match); Best Regards Matti -- Matti Vaittinen, Linux device drivers ROHM Semiconductors, Finland SWDC Kiviharjunlenkki 1E 90220 OULU FINLAND ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ Simon says - in Latin please. "non cogito me" dixit Rene Descarte, deinde evanescavit (Thanks for the translation Simon)
On Thu, 2020-11-05 at 08:46 +0200, Matti Vaittinen wrote: > Morning Lee, > > Thanks for taking a look at this :) I see most of the comments being > valid. There's two I would like to clarify though... > > On Wed, 2020-11-04 at 15:51 +0000, Lee Jones wrote: > > On Wed, 28 Oct 2020, Matti Vaittinen wrote: > > > > > Add core support for ROHM BD9576MUF and BD9573MUF PMICs which are > > > mainly used to power the R-Car series processors. > > > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com > > > > > > > --- > > > + unsigned int chip_type; > > > + > > > + chip_type = (unsigned int)(uintptr_t) > > > + of_device_get_match_data(&i2c->dev); > > > > Not overly keen on this casting. > > > > Why not just leave it as (uintptr_t)? > > I didn't do so because on x86_64 the address width is probably 64 > bits > whereas the unsigned int is likely to be 32 bits. So the assignment > will crop half of the value. It does not really matter as values are > small - but I would be surprized if no compilers/analyzers emitted a > warning. > > I must admit I am not 100% sure though. I sure can change this if you > know it better? > > > What happens when you don't cast to (uintptr_t) first? > > On some systems at least the gcc will warn: > > warning: cast from pointer to integer of different size [-Wpointer- > to-int-cast] > > I am pretty sure I did end up this double casting via trial and error > :) > + > > > +static const struct of_device_id bd957x_of_match[] = { > > > + { > > > + .compatible = "rohm,bd9576", > > > + .data = (void *)ROHM_CHIP_TYPE_BD9576, > > > + }, > > > + { > > > > You could put the 2 lines above on a single line. > > Braces? I put braces on separate lines on purpose. Been doing this > after we had this discussion: > > https://lore.kernel.org/lkml/20180705055226.GJ496@dell/ > https://lore.kernel.org/lkml/20180706070559.GW496@dell/ > > ;) > > I can change it if you wishfeel it is important - not a point I feel > like fighting over ;) > Ah. I guess you meant: static const struct of_device_id bd957x_of_match[] = { { .compatible = "rohm,bd9576", .data = (void *)ROHM_CHIP_TYPE_BD9576, }, { .compatible = "rohm,bd9573", .data = (void *)ROHM_CHIP_TYPE_BD9573, }, {}, }; Feeling "little bit" stupid... :rolleyes: > > > + .compatible = "rohm,bd9573", > > > + .data = (void *)ROHM_CHIP_TYPE_BD9573, > > > + }, > > > + { }, > > > +}; > > > +MODULE_DEVICE_TABLE(of, bd957x_of_match); > > Best Regards > Matti > > -- > Matti Vaittinen, Linux device drivers > ROHM Semiconductors, Finland > SWDC > Kiviharjunlenkki 1E > 90220 OULU > FINLAND > > ~~~ "I don't think so," said Rene Descartes. Just then he vanished > ~~~ > > Simon says - in Latin please. > "non cogito me" dixit Rene Descarte, deinde evanescavit > > (Thanks for the translation Simon) >
On Thu, 05 Nov 2020, Vaittinen, Matti wrote: > > On Thu, 2020-11-05 at 08:46 +0200, Matti Vaittinen wrote: > > Morning Lee, > > > > Thanks for taking a look at this :) I see most of the comments being > > valid. There's two I would like to clarify though... > > > > On Wed, 2020-11-04 at 15:51 +0000, Lee Jones wrote: > > > On Wed, 28 Oct 2020, Matti Vaittinen wrote: > > > > > > > Add core support for ROHM BD9576MUF and BD9573MUF PMICs which are > > > > mainly used to power the R-Car series processors. > > > > > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com > > > > > > > > > --- > > > > + unsigned int chip_type; > > > > + > > > > + chip_type = (unsigned int)(uintptr_t) > > > > + of_device_get_match_data(&i2c->dev); > > > > > > Not overly keen on this casting. > > > > > > Why not just leave it as (uintptr_t)? > > > > I didn't do so because on x86_64 the address width is probably 64 > > bits > > whereas the unsigned int is likely to be 32 bits. So the assignment > > will crop half of the value. It does not really matter as values are > > small - but I would be surprized if no compilers/analyzers emitted a > > warning. > > > > I must admit I am not 100% sure though. I sure can change this if you > > know it better? What if you used 'long', which I believe changed with the architecture's bus width in Linux? > > > What happens when you don't cast to (uintptr_t) first? > > > > On some systems at least the gcc will warn: > > > warning: cast from pointer to integer of different size [-Wpointer- > > to-int-cast] > > > > I am pretty sure I did end up this double casting via trial and error > > :) It's not uncommon. :) > > > > +static const struct of_device_id bd957x_of_match[] = { > > > > + { > > > > + .compatible = "rohm,bd9576", > > > > + .data = (void *)ROHM_CHIP_TYPE_BD9576, > > > > + }, > > > > + { > > > > > > You could put the 2 lines above on a single line. > > > > Braces? I put braces on separate lines on purpose. Been doing this > > after we had this discussion: > > > > https://lore.kernel.org/lkml/20180705055226.GJ496@dell/ > > https://lore.kernel.org/lkml/20180706070559.GW496@dell/ > > > > ;) > > > > I can change it if you wishfeel it is important - not a point I feel > > like fighting over ;) > > > > Ah. I guess you meant: > static const struct of_device_id bd957x_of_match[] = { > { .compatible = "rohm,bd9576", .data = (void *)ROHM_CHIP_TYPE_BD9576, }, > { .compatible = "rohm,bd9573", .data = (void *)ROHM_CHIP_TYPE_BD9573, }, > {}, > }; This would be better, yes.
On Wed, 04 Nov 2020, Lee Jones wrote: > On Wed, 28 Oct 2020, Matti Vaittinen wrote: > > > Add core support for ROHM BD9576MUF and BD9573MUF PMICs which are > > mainly used to power the R-Car series processors. > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > --- > > drivers/mfd/Kconfig | 11 +++ > > drivers/mfd/Makefile | 1 + > > drivers/mfd/rohm-bd9576.c | 130 +++++++++++++++++++++++++++++++ > > include/linux/mfd/rohm-bd957x.h | 59 ++++++++++++++ > > include/linux/mfd/rohm-generic.h | 2 + > > 5 files changed, 203 insertions(+) > > create mode 100644 drivers/mfd/rohm-bd9576.c > > create mode 100644 include/linux/mfd/rohm-bd957x.h [...] > > +static const struct regmap_range volatile_ranges[] = { > > + { > > + .range_min = BD957X_REG_SMRB_ASSERT, > > + .range_max = BD957X_REG_SMRB_ASSERT, > > + }, > > + { > > The way you space your braces is not consistent. > > > + .range_min = BD957X_REG_PMIC_INTERNAL_STAT, > > + .range_max = BD957X_REG_PMIC_INTERNAL_STAT, > > + }, > > + { > > + .range_min = BD957X_REG_INT_THERM_STAT, > > + .range_max = BD957X_REG_INT_THERM_STAT, > > + }, > > + { > > + .range_min = BD957X_REG_INT_OVP_STAT, > > + .range_max = BD957X_REG_INT_SYS_STAT, > > + }, { > > + .range_min = BD957X_REG_INT_MAIN_STAT, > > + .range_max = BD957X_REG_INT_MAIN_STAT, > > + }, > > +}; Don't forget about this. I would prefer to have the braces on the same line (even if it means you have to change an extra line when editing), but I'm not 100% dead set on it. Consistency however, I am.
On Thu, 2020-11-05 at 08:21 +0000, Lee Jones wrote: > On Thu, 05 Nov 2020, Vaittinen, Matti wrote: > > > On Thu, 2020-11-05 at 08:46 +0200, Matti Vaittinen wrote: > > > Morning Lee, > > > > > > Thanks for taking a look at this :) I see most of the comments > > > being > > > valid. There's two I would like to clarify though... > > > > > > On Wed, 2020-11-04 at 15:51 +0000, Lee Jones wrote: > > > > On Wed, 28 Oct 2020, Matti Vaittinen wrote: > > > > > > > > > Add core support for ROHM BD9576MUF and BD9573MUF PMICs which > > > > > are > > > > > mainly used to power the R-Car series processors. > > > > > > > > > > Signed-off-by: Matti Vaittinen < > > > > > matti.vaittinen@fi.rohmeurope.com > > > > > --- > > > > > + unsigned int chip_type; > > > > > + > > > > > + chip_type = (unsigned int)(uintptr_t) > > > > > + of_device_get_match_data(&i2c->dev); > > > > > > > > Not overly keen on this casting. > > > > > > > > Why not just leave it as (uintptr_t)? > > > > > > I didn't do so because on x86_64 the address width is probably 64 > > > bits > > > whereas the unsigned int is likely to be 32 bits. So the > > > assignment > > > will crop half of the value. It does not really matter as values > > > are > > > small - but I would be surprized if no compilers/analyzers > > > emitted a > > > warning. > > > > > > I must admit I am not 100% sure though. I sure can change this if > > > you > > > know it better? > > What if you used 'long', which I believe changed with the > architecture's bus width in Linux? I think this is exactly what uintptr_t was created for. To provide type which assures a pointer conversion to integer and back works. I guess I can change the unsigned int chip_type; to uintptr_t and get away with single cast if it looks better to you. For me the double cast does not look that bad when it allows use of native int size variable - but in this case it's really just a matter of taste. Both should work fine. I'll cook v5. --Matti
On Thu, 05 Nov 2020, Vaittinen, Matti wrote: > > On Thu, 2020-11-05 at 08:21 +0000, Lee Jones wrote: > > On Thu, 05 Nov 2020, Vaittinen, Matti wrote: > > > > > On Thu, 2020-11-05 at 08:46 +0200, Matti Vaittinen wrote: > > > > Morning Lee, > > > > > > > > Thanks for taking a look at this :) I see most of the comments > > > > being > > > > valid. There's two I would like to clarify though... > > > > > > > > On Wed, 2020-11-04 at 15:51 +0000, Lee Jones wrote: > > > > > On Wed, 28 Oct 2020, Matti Vaittinen wrote: > > > > > > > > > > > Add core support for ROHM BD9576MUF and BD9573MUF PMICs which > > > > > > are > > > > > > mainly used to power the R-Car series processors. > > > > > > > > > > > > Signed-off-by: Matti Vaittinen < > > > > > > matti.vaittinen@fi.rohmeurope.com > > > > > > --- > > > > > > + unsigned int chip_type; > > > > > > + > > > > > > + chip_type = (unsigned int)(uintptr_t) > > > > > > + of_device_get_match_data(&i2c->dev); > > > > > > > > > > Not overly keen on this casting. > > > > > > > > > > Why not just leave it as (uintptr_t)? > > > > > > > > I didn't do so because on x86_64 the address width is probably 64 > > > > bits > > > > whereas the unsigned int is likely to be 32 bits. So the > > > > assignment > > > > will crop half of the value. It does not really matter as values > > > > are > > > > small - but I would be surprized if no compilers/analyzers > > > > emitted a > > > > warning. > > > > > > > > I must admit I am not 100% sure though. I sure can change this if > > > > you > > > > know it better? > > > > What if you used 'long', which I believe changed with the > > architecture's bus width in Linux? > > I think this is exactly what uintptr_t was created for. To provide type > which assures a pointer conversion to integer and back works. > > I guess I can change the > > unsigned int chip_type; > > to uintptr_t and get away with single cast if it looks better to you. > For me the double cast does not look that bad when it allows use of > native int size variable - but in this case it's really just a matter > of taste. Both should work fine. I do see people casting to uintptr and placing the result into a long.
On Thu, 2020-11-05 at 08:23 +0000, Lee Jones wrote: > On Wed, 04 Nov 2020, Lee Jones wrote: > > > On Wed, 28 Oct 2020, Matti Vaittinen wrote: > > > > > Add core support for ROHM BD9576MUF and BD9573MUF PMICs which are > > > mainly used to power the R-Car series processors. > > > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com > > > > > > > --- > > > drivers/mfd/Kconfig | 11 +++ > > > drivers/mfd/Makefile | 1 + > > > drivers/mfd/rohm-bd9576.c | 130 > > > +++++++++++++++++++++++++++++++ > > > include/linux/mfd/rohm-bd957x.h | 59 ++++++++++++++ > > > include/linux/mfd/rohm-generic.h | 2 + > > > 5 files changed, 203 insertions(+) > > > create mode 100644 drivers/mfd/rohm-bd9576.c > > > create mode 100644 include/linux/mfd/rohm-bd957x.h > > [...] > > > > +static const struct regmap_range volatile_ranges[] = { > > > + { > > > + .range_min = BD957X_REG_SMRB_ASSERT, > > > + .range_max = BD957X_REG_SMRB_ASSERT, > > > + }, > > > + { > > > > The way you space your braces is not consistent. > > > > > + .range_min = BD957X_REG_PMIC_INTERNAL_STAT, > > > + .range_max = BD957X_REG_PMIC_INTERNAL_STAT, > > > + }, > > > + { > > > + .range_min = BD957X_REG_INT_THERM_STAT, > > > + .range_max = BD957X_REG_INT_THERM_STAT, > > > + }, > > > + { > > > + .range_min = BD957X_REG_INT_OVP_STAT, > > > + .range_max = BD957X_REG_INT_SYS_STAT, > > > + }, { > > > + .range_min = BD957X_REG_INT_MAIN_STAT, > > > + .range_max = BD957X_REG_INT_MAIN_STAT, > > > + }, > > > +}; > > Don't forget about this. > > I would prefer to have the braces on the same line (even if it means > you have to change an extra line when editing), but I'm not 100% dead > set on it. Consistency however, I am. > I won't forget. I intended to write that I was Ok with all the other comments. Maybe I forgot though. Anyways, I'll fix the inconsistency - thanks for pointing it out! --Matti
On Thu, 2020-11-05 at 08:58 +0000, Lee Jones wrote: > On Thu, 05 Nov 2020, Vaittinen, Matti wrote: > > > On Thu, 2020-11-05 at 08:21 +0000, Lee Jones wrote: > > > On Thu, 05 Nov 2020, Vaittinen, Matti wrote: > > > > > > > On Thu, 2020-11-05 at 08:46 +0200, Matti Vaittinen wrote: > > > > > Morning Lee, > > > > > > > > > > Thanks for taking a look at this :) I see most of the > > > > > comments > > > > > being > > > > > valid. There's two I would like to clarify though... > > > > > > > > > > On Wed, 2020-11-04 at 15:51 +0000, Lee Jones wrote: > > > > > > On Wed, 28 Oct 2020, Matti Vaittinen wrote: > > > > > > > > > > > > > Add core support for ROHM BD9576MUF and BD9573MUF PMICs > > > > > > > which > > > > > > > are > > > > > > > mainly used to power the R-Car series processors. > > > > > > > > > > > > > > Signed-off-by: Matti Vaittinen < > > > > > > > matti.vaittinen@fi.rohmeurope.com > > > > > > > --- > > > > > > > + unsigned int chip_type; > > > > > > > + > > > > > > > + chip_type = (unsigned int)(uintptr_t) > > > > > > > + of_device_get_match_data(&i2c->dev); > > > > > > > > > > > > Not overly keen on this casting. > > > > > > > > > > > > Why not just leave it as (uintptr_t)? > > > > > > > > > > I didn't do so because on x86_64 the address width is > > > > > probably 64 > > > > > bits > > > > > whereas the unsigned int is likely to be 32 bits. So the > > > > > assignment > > > > > will crop half of the value. It does not really matter as > > > > > values > > > > > are > > > > > small - but I would be surprized if no compilers/analyzers > > > > > emitted a > > > > > warning. > > > > > > > > > > I must admit I am not 100% sure though. I sure can change > > > > > this if > > > > > you > > > > > know it better? > > > > > > What if you used 'long', which I believe changed with the > > > architecture's bus width in Linux? > > > > I think this is exactly what uintptr_t was created for. To provide > > type > > which assures a pointer conversion to integer and back works. > > > > I guess I can change the > > > > unsigned int chip_type; > > > > to uintptr_t and get away with single cast if it looks better to > > you. > > For me the double cast does not look that bad when it allows use of > > native int size variable - but in this case it's really just a > > matter > > of taste. Both should work fine. > > I do see people casting to uintptr and placing the result into a > long. That should work because long is same size as address on architectures supported by Linux. But it still does not "feel correct" to me. Why bothering to use the uintptr_t in first place then? So ... I believe it should *work* on all currently supported architectures if I do: unsigned long chip_type; chip_type = (unsigned long)of_device_get_match_data(&i2c->dev); although fact it works does not make it *right* :] But casting to something (uintptr_t) and then assigning to something else (unsigned long) just trusting that it *works* does feel slightly fishy when we could use same type for variable and cast. But again - if unsigned long chip_type; ... chip_type = (unsigned long)of_device_get_match_data(&i2c->dev); is what you prefer - I can do that too. Especially in this case where I expect the highest number to definitely stay less than three digits. If we some day hit to architecture where addresses are not of same size as longs, then this is just one minor conversion to do... --Matti