RE: [PATCH 6/6] mfd: da9063: Add DA9063L support
On 24 May 2018 15:51 Marek Vasut wrote: Hi Marek, > To: Steve Twiss <stwiss.opensou...@diasemi.com>; linux-ker...@vger.kernel.org > Cc: Marek Vasut <marek.vasut+rene...@gmail.com>; Geert Uytterhoeven > <geert+rene...@glider.be>; Lee Jones <lee.jo...@linaro.org>; Mark Brown > <broo...@kernel.org>; Wolfram Sang <wsa+rene...@sang-engineering.com>; > linux-renesas-soc@vger.kernel.org > Subject: Re: [PATCH 6/6] mfd: da9063: Add DA9063L support > > On 05/24/2018 02:32 PM, Steve Twiss wrote: > > On 24 May 2018 @ 12:49 Steve Twiss wrote: > >>> On 23 May 2018 12:43 Marek Vasut wrote, > >>> > >>> To: linux-ker...@vger.kernel.org > >>> Subject: [PATCH 6/6] mfd: da9063: Add DA9063L support > >>> > >>> Add support for DA9063L, which is a reduced variant of the DA9063 with > >>> less regulators and without RTC. > >>> > >> > >> There's potentially more to this file. Without an RTC the regmap > >> access tables would change and the usual DA9063 (BB silicon) tables would > >> become invalid. > >> The tables for da9063_bb_readable_ranges, da9063_bb_writeable_ranges, > >> da9063_bb_volatile_ranges, would need to be updated for DA9063L, if a new > >> chip model was needed. > >> > >> The new ranges would be this (see below), and would remove any RTC > >> accesses in the new chip model. > >> > >> static const struct regmap_range da9063l_bb_readable_ranges[] = { > >>{ > >>.range_min = DA9063_REG_PAGE_CON, > >>.range_max = DA9063_REG_MON_A10_RES, > >>}, { > >>.range_min = DA9063_REG_SEQ, > >>.range_max = DA9063_REG_ID_32_31, > >>}, { > >>.range_min = DA9063_REG_SEQ_A, > >>.range_max = DA9063_REG_AUTO3_LOW, > >>}, { > >>.range_min = DA9063_REG_T_OFFSET, > >>.range_max = DA9063_BB_REG_GP_ID_19, > >>}, { > >>.range_min = DA9063_REG_CHIP_ID, > >>.range_max = DA9063_REG_CHIP_VARIANT, > >>}, > >> }; > >> > >> static const struct regmap_range da9063l_bb_writeable_ranges[] = { > >>{ > >>.range_min = DA9063_REG_PAGE_CON, > >>.range_max = DA9063_REG_PAGE_CON, > >>}, { > >>.range_min = DA9063_REG_FAULT_LOG, > >>.range_max = DA9063_REG_VSYS_MON, > >>}, { > >>.range_min = DA9063_REG_SEQ, > >>.range_max = DA9063_REG_ID_32_31, > >>}, { > >>.range_min = DA9063_REG_SEQ_A, > >>.range_max = DA9063_REG_AUTO3_LOW, > >>}, { > >>.range_min = DA9063_REG_CONFIG_I, > >>.range_max = DA9063_BB_REG_MON_REG_4, > >>}, { > >>.range_min = DA9063_BB_REG_GP_ID_0, > >>.range_max = DA9063_BB_REG_GP_ID_19, > >>}, > >> }; > >> > >> static const struct regmap_range da9063l_bb_volatile_ranges[] = { > >>{ > >>.range_min = DA9063_REG_PAGE_CON, > >>.range_max = DA9063_REG_EVENT_D, > >>}, { > >>.range_min = DA9063_REG_CONTROL_A, > >>.range_max = DA9063_REG_CONTROL_B, > >>}, { > >>.range_min = DA9063_REG_CONTROL_E, > >>.range_max = DA9063_REG_CONTROL_F, > >>}, { > >>.range_min = DA9063_REG_BCORE2_CONT, > >>.range_max = DA9063_REG_LDO11_CONT, > >>}, { > >>.range_min = DA9063_REG_DVC_1, > >>.range_max = DA9063_REG_ADC_MAN, > >>}, { > >>.range_min = DA9063_REG_ADC_RES_L, > >>.range_max = DA9063_REG_MON_A10_RES, > >>}, { > >>.range_min = DA9063_REG_SEQ, > >>.range_max = DA9063_REG_SEQ, > >>}, { > >>.range_min = DA9063_REG_EN_32K, > >>.range_max = DA9063_REG_EN_32K, > >>}, { > >>.range_min = DA9063_BB_REG_MON_REG_5, > >>.range_max = DA9063_BB_REG_MON_REG_6, > >>}, > >> }; > >> > >> However this is a larger and more wide-ranging change compared to the > >> one proposed by Marek, and would require other alterations to fit > >> this in. Also I'm undecided to what it would r
RE: [PATCH 3/6] mfd: da9063: Add DA9063L type
On 23 May 2018 12:42 Marek Vasut wrote: > To: linux-ker...@vger.kernel.org > Cc: Marek Vasut <marek.vasut+rene...@gmail.com>; Geert Uytterhoeven > <geert+rene...@glider.be>; Lee Jones <lee.jo...@linaro.org>; Mark Brown > <broo...@kernel.org>; Steve Twiss <stwiss.opensou...@diasemi.com>; Wolfram > Sang <wsa+rene...@sang-engineering.com>; linux-renesas-soc@vger.kernel.org > Subject: [PATCH 3/6] mfd: da9063: Add DA9063L type > > Add type for DA9063L, which is a reduced variant of the DA9063 without RTC > block and with less regulators. > > Signed-off-by: Marek Vasut <marek.vasut+rene...@gmail.com> > Cc: Geert Uytterhoeven <geert+rene...@glider.be> > Cc: Lee Jones <lee.jo...@linaro.org> > Cc: Mark Brown <broo...@kernel.org> > Cc: Steve Twiss <stwiss.opensou...@diasemi.com> > Cc: Wolfram Sang <wsa+rene...@sang-engineering.com> > Cc: linux-renesas-soc@vger.kernel.org > --- > include/linux/mfd/da9063/core.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/linux/mfd/da9063/core.h > b/include/linux/mfd/da9063/core.h index eb234582dcb2..c3e917dd1860 100644 > --- a/include/linux/mfd/da9063/core.h > +++ b/include/linux/mfd/da9063/core.h > @@ -33,6 +33,7 @@ > > enum da9063_type { > PMIC_TYPE_DA9063 = 0, > + PMIC_TYPE_DA9063L, > }; > > enum da9063_variant_codes { > -- > 2.16.2 Acked-by: Steve Twiss <stwiss.opensou...@diasemi.com> Regards, Steve
RE: [PATCH 4/6] mfd: da9063: Disallow RTC on DA9063L
Thanks Marek, On 23 May 2018 12:42 Marek Vasut wrote, > To: linux-ker...@vger.kernel.org > Cc: Marek Vasut <marek.vasut+rene...@gmail.com>; Geert Uytterhoeven > <geert+rene...@glider.be>; Lee Jones <lee.jo...@linaro.org>; Mark Brown > <broo...@kernel.org>; Steve Twiss <stwiss.opensou...@diasemi.com>; Wolfram > Sang <wsa+rene...@sang-engineering.com>; linux-renesas-soc@vger.kernel.org > Subject: [PATCH 4/6] mfd: da9063: Disallow RTC on DA9063L > > The DA9063L does not contain RTC block, unlike the full DA9063. > Do not allow binding RTC driver on this variant of the chip. > > Signed-off-by: Marek Vasut <marek.vasut+rene...@gmail.com> > Cc: Geert Uytterhoeven <geert+rene...@glider.be> > Cc: Lee Jones <lee.jo...@linaro.org> > Cc: Mark Brown <broo...@kernel.org> > Cc: Steve Twiss <stwiss.opensou...@diasemi.com> > Cc: Wolfram Sang <wsa+rene...@sang-engineering.com> > Cc: linux-renesas-soc@vger.kernel.org > --- > drivers/mfd/da9063-core.c | 18 +++--- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c index > 7360b76b4f72..263c83006413 100644 > --- a/drivers/mfd/da9063-core.c > +++ b/drivers/mfd/da9063-core.c > @@ -101,14 +101,14 @@ static const struct mfd_cell da9063_devs[] = { > .of_compatible = "dlg,da9063-onkey", > }, > { > + .name = DA9063_DRVNAME_VIBRATION, > + }, > + { /* Only present on DA9063 , not on DA9063L */ > .name = DA9063_DRVNAME_RTC, > .num_resources = ARRAY_SIZE(da9063_rtc_resources), > .resources = da9063_rtc_resources, > .of_compatible = "dlg,da9063-rtc", > }, > - { > - .name = DA9063_DRVNAME_VIBRATION, > - }, > }; > > static int da9063_clear_fault_log(struct da9063 *da9063) @@ -163,7 +163,7 @@ > int da9063_device_init(struct da9063 *da9063, unsigned int irq) { > struct da9063_pdata *pdata = da9063->dev->platform_data; > int model, variant_id, variant_code; > - int ret; > + int da9063_devs_len, ret; > > ret = da9063_clear_fault_log(da9063); > if (ret < 0) > @@ -225,9 +225,13 @@ int da9063_device_init(struct da9063 *da9063, unsigned > int irq) > > da9063->irq_base = regmap_irq_chip_get_base(da9063->regmap_irq); > > - ret = mfd_add_devices(da9063->dev, -1, da9063_devs, > - ARRAY_SIZE(da9063_devs), NULL, da9063->irq_base, > - NULL); > + da9063_devs_len = ARRAY_SIZE(da9063_devs); > + /* RTC, the last device in the list, is only present on DA9063 */ > + if (da9063->type == PMIC_TYPE_DA9063L) > + da9063_devs_len -= 1; > + > + ret = mfd_add_devices(da9063->dev, -1, da9063_devs, da9063_devs_len, > + NULL, da9063->irq_base, NULL); > if (ret) > dev_err(da9063->dev, "Cannot add MFD cells\n"); > > -- > 2.16.2 MFD cells definitely has less impact than regmap_range and regmap_irq. I agree, there's no point in having a completely new static const struct mfd_cell da9063l_devs[] = { ... } for DA9063L Acked-by: Steve Twiss <stwiss.opensou...@diasemi.com> Regards, Steve
RE: [PATCH 6/6] mfd: da9063: Add DA9063L support
Hi Marek, On 24 May 2018 @ 12:49 Steve Twiss wrote: > To: Marek Vasut <marek.va...@gmail.com>; linux-ker...@vger.kernel.org > Cc: Marek Vasut <marek.vasut+rene...@gmail.com>; Geert Uytterhoeven > <geert+rene...@glider.be>; Lee Jones <lee.jo...@linaro.org>; Mark Brown > <broo...@kernel.org>; Steve Twiss <stwiss.opensou...@diasemi.com>; Wolfram > Sang <wsa+rene...@sang-engineering.com>; linux-renesas-soc@vger.kernel.org > Subject: RE: [PATCH 6/6] mfd: da9063: Add DA9063L support > > Thanks Marek, > > > On 23 May 2018 12:43 Marek Vasut wrote, > > > > To: linux-ker...@vger.kernel.org > > Cc: Marek Vasut <marek.vasut+rene...@gmail.com>; Geert Uytterhoeven > > <geert+rene...@glider.be>; Lee Jones <lee.jo...@linaro.org>; > > Mark Brown <broo...@kernel.org>; Steve Twiss > > <stwiss.opensou...@diasemi.com>; Wolfram Sang > > <wsa+rene...@sang-engineering.com>; linux-renesas-soc@vger.kernel.org > > Subject: [PATCH 6/6] mfd: da9063: Add DA9063L support > > > > Add support for DA9063L, which is a reduced variant of the DA9063 with less > > regulators and without RTC. > > > > There's potentially more to this file. Without an RTC the regmap access > tables would change > and the usual DA9063 (BB silicon) tables would become invalid. > The tables for da9063_bb_readable_ranges, da9063_bb_writeable_ranges, > da9063_bb_volatile_ranges, > would need to be updated for DA9063L, if a new chip model was needed. > > The new ranges would be this (see below), and would remove any RTC accesses > in the new chip model. > > static const struct regmap_range da9063l_bb_readable_ranges[] = { > { > .range_min = DA9063_REG_PAGE_CON, > .range_max = DA9063_REG_MON_A10_RES, > }, { > .range_min = DA9063_REG_SEQ, > .range_max = DA9063_REG_ID_32_31, > }, { > .range_min = DA9063_REG_SEQ_A, > .range_max = DA9063_REG_AUTO3_LOW, > }, { > .range_min = DA9063_REG_T_OFFSET, > .range_max = DA9063_BB_REG_GP_ID_19, > }, { > .range_min = DA9063_REG_CHIP_ID, > .range_max = DA9063_REG_CHIP_VARIANT, > }, > }; > > static const struct regmap_range da9063l_bb_writeable_ranges[] = { > { > .range_min = DA9063_REG_PAGE_CON, > .range_max = DA9063_REG_PAGE_CON, > }, { > .range_min = DA9063_REG_FAULT_LOG, > .range_max = DA9063_REG_VSYS_MON, > }, { > .range_min = DA9063_REG_SEQ, > .range_max = DA9063_REG_ID_32_31, > }, { > .range_min = DA9063_REG_SEQ_A, > .range_max = DA9063_REG_AUTO3_LOW, > }, { > .range_min = DA9063_REG_CONFIG_I, > .range_max = DA9063_BB_REG_MON_REG_4, > }, { > .range_min = DA9063_BB_REG_GP_ID_0, > .range_max = DA9063_BB_REG_GP_ID_19, > }, > }; > > static const struct regmap_range da9063l_bb_volatile_ranges[] = { > { > .range_min = DA9063_REG_PAGE_CON, > .range_max = DA9063_REG_EVENT_D, > }, { > .range_min = DA9063_REG_CONTROL_A, > .range_max = DA9063_REG_CONTROL_B, > }, { > .range_min = DA9063_REG_CONTROL_E, > .range_max = DA9063_REG_CONTROL_F, > }, { > .range_min = DA9063_REG_BCORE2_CONT, > .range_max = DA9063_REG_LDO11_CONT, > }, { > .range_min = DA9063_REG_DVC_1, > .range_max = DA9063_REG_ADC_MAN, > }, { > .range_min = DA9063_REG_ADC_RES_L, > .range_max = DA9063_REG_MON_A10_RES, > }, { > .range_min = DA9063_REG_SEQ, > .range_max = DA9063_REG_SEQ, > }, { > .range_min = DA9063_REG_EN_32K, > .range_max = DA9063_REG_EN_32K, > }, { > .range_min = DA9063_BB_REG_MON_REG_5, > .range_max = DA9063_BB_REG_MON_REG_6, > }, > }; > > However this is a larger and more wide-ranging change compared to the one > proposed by Marek, > and would require other alterations to fit this in. Also I'm undecided to > what it would really add > apart from a new chip model: I have been told accessing the DA9063 RTC > register locations has no > effect in the DA9063L. Looking at this further, there is also a new IRQ regmap. Again this comes down to whether a full chip model is needed or not. If not, then the IRQ map does not n
RE: [PATCH 1/6] mfd: da9063: Rename PMIC_DA9063 to PMIC_CHIP_ID_DA9063
Hi Marek, On 23 May 2018 12:42 Marek Vasut wrote: > To: linux-ker...@vger.kernel.org > Cc: Marek Vasut <marek.vasut+rene...@gmail.com>; Geert Uytterhoeven > <geert+rene...@glider.be>; Lee Jones <lee.jo...@linaro.org>; Mark Brown > <broo...@kernel.org>; Steve Twiss <stwiss.opensou...@diasemi.com>; Wolfram > Sang <wsa+rene...@sang-engineering.com>; linux-renesas-soc@vger.kernel.org > Subject: [PATCH 1/6] mfd: da9063: Rename PMIC_DA9063 to PMIC_CHIP_ID_DA9063 > > The PMIC_DA9063 is a complete misnomer, it denotes the value of the > DA9063 chip ID register, so rename it as such. It is also the value of chip > ID register of DA9063L though, so drop the enum as all the > DA9063 "models" share the same chip ID and thus the distinction will have to > be made using DT or otherwise. > > Signed-off-by: Marek Vasut <marek.vasut+rene...@gmail.com> > Cc: Geert Uytterhoeven <geert+rene...@glider.be> > Cc: Lee Jones <lee.jo...@linaro.org> > Cc: Mark Brown <broo...@kernel.org> > Cc: Steve Twiss <stwiss.opensou...@diasemi.com> > Cc: Wolfram Sang <wsa+rene...@sang-engineering.com> > Cc: linux-renesas-soc@vger.kernel.org > --- > drivers/mfd/da9063-core.c| 2 +- > drivers/mfd/da9063-i2c.c | 2 +- > drivers/regulator/da9063-regulator.c | 2 +- > include/linux/mfd/da9063/core.h | 4 +--- > 4 files changed, 4 insertions(+), 6 deletions(-) > > diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c index > 6c2870d4e754..00b3caee4e21 100644 > --- a/drivers/mfd/da9063-core.c > +++ b/drivers/mfd/da9063-core.c > @@ -192,7 +192,7 @@ int da9063_device_init(struct da9063 *da9063, unsigned > int irq) > dev_err(da9063->dev, "Cannot read chip model id.\n"); > return -EIO; > } > - if (model != PMIC_DA9063) { > + if (model != PMIC_CHIP_ID_DA9063) { > dev_err(da9063->dev, "Invalid chip model id: 0x%02x\n", model); > return -ENODEV; > } > diff --git a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c index > 981805a2c521..7f84030c8d53 100644 > --- a/drivers/mfd/da9063-i2c.c > +++ b/drivers/mfd/da9063-i2c.c > @@ -280,7 +280,7 @@ static int da9063_i2c_remove(struct i2c_client *i2c) } > > static const struct i2c_device_id da9063_i2c_id[] = { > - {"da9063", PMIC_DA9063}, > + { "da9063", PMIC_CHIP_ID_DA9063 }, > {}, > }; > MODULE_DEVICE_TABLE(i2c, da9063_i2c_id); diff --git > a/drivers/regulator/da9063-regulator.c b/drivers/regulator/da9063-regulator.c > index 6a8f9cd69f52..87c884ae0064 100644 > --- a/drivers/regulator/da9063-regulator.c > +++ b/drivers/regulator/da9063-regulator.c > @@ -585,7 +585,7 @@ static struct da9063_dev_model regulators_models[] = { > { > .regulator_info = da9063_regulator_info, > .n_regulators = ARRAY_SIZE(da9063_regulator_info), > - .dev_model = PMIC_DA9063, > + .dev_model = PMIC_CHIP_ID_DA9063, > }, > { } > }; > diff --git a/include/linux/mfd/da9063/core.h > b/include/linux/mfd/da9063/core.h index f3ae65db4c86..664f650d0086 100644 > --- a/include/linux/mfd/da9063/core.h > +++ b/include/linux/mfd/da9063/core.h > @@ -29,9 +29,7 @@ > #define DA9063_DRVNAME_RTC "da9063-rtc" > #define DA9063_DRVNAME_VIBRATION "da9063-vibration" > > -enum da9063_models { > - PMIC_DA9063 = 0x61, > -}; > +#define PMIC_CHIP_ID_DA9063 0x61 > > enum da9063_variant_codes { > PMIC_DA9063_AD = 0x3, > -- > 2.16.2 Acked-by: Steve Twiss <stwiss.opensou...@diasemi.com> Regards, Steve
RE: [PATCH 6/6] mfd: da9063: Add DA9063L support
Thanks Marek, > On 23 May 2018 12:43 Marek Vasut wrote, > > To: linux-ker...@vger.kernel.org > Cc: Marek Vasut <marek.vasut+rene...@gmail.com>; Geert Uytterhoeven > <geert+rene...@glider.be>; > Lee Jones <lee.jo...@linaro.org>; Mark Brown <broo...@kernel.org>; Steve > Twiss <stwiss.opensou...@diasemi.com>; > Wolfram Sang <wsa+rene...@sang-engineering.com>; > linux-renesas-soc@vger.kernel.org > Subject: [PATCH 6/6] mfd: da9063: Add DA9063L support > > Add support for DA9063L, which is a reduced variant of the DA9063 with less > regulators and without RTC. > There's potentially more to this file. Without an RTC the regmap access tables would change and the usual DA9063 (BB silicon) tables would become invalid. The tables for da9063_bb_readable_ranges, da9063_bb_writeable_ranges, da9063_bb_volatile_ranges, would need to be updated for DA9063L, if a new chip model was needed. The new ranges would be this (see below), and would remove any RTC accesses in the new chip model. static const struct regmap_range da9063l_bb_readable_ranges[] = { { .range_min = DA9063_REG_PAGE_CON, .range_max = DA9063_REG_MON_A10_RES, }, { .range_min = DA9063_REG_SEQ, .range_max = DA9063_REG_ID_32_31, }, { .range_min = DA9063_REG_SEQ_A, .range_max = DA9063_REG_AUTO3_LOW, }, { .range_min = DA9063_REG_T_OFFSET, .range_max = DA9063_BB_REG_GP_ID_19, }, { .range_min = DA9063_REG_CHIP_ID, .range_max = DA9063_REG_CHIP_VARIANT, }, }; static const struct regmap_range da9063l_bb_writeable_ranges[] = { { .range_min = DA9063_REG_PAGE_CON, .range_max = DA9063_REG_PAGE_CON, }, { .range_min = DA9063_REG_FAULT_LOG, .range_max = DA9063_REG_VSYS_MON, }, { .range_min = DA9063_REG_SEQ, .range_max = DA9063_REG_ID_32_31, }, { .range_min = DA9063_REG_SEQ_A, .range_max = DA9063_REG_AUTO3_LOW, }, { .range_min = DA9063_REG_CONFIG_I, .range_max = DA9063_BB_REG_MON_REG_4, }, { .range_min = DA9063_BB_REG_GP_ID_0, .range_max = DA9063_BB_REG_GP_ID_19, }, }; static const struct regmap_range da9063l_bb_volatile_ranges[] = { { .range_min = DA9063_REG_PAGE_CON, .range_max = DA9063_REG_EVENT_D, }, { .range_min = DA9063_REG_CONTROL_A, .range_max = DA9063_REG_CONTROL_B, }, { .range_min = DA9063_REG_CONTROL_E, .range_max = DA9063_REG_CONTROL_F, }, { .range_min = DA9063_REG_BCORE2_CONT, .range_max = DA9063_REG_LDO11_CONT, }, { .range_min = DA9063_REG_DVC_1, .range_max = DA9063_REG_ADC_MAN, }, { .range_min = DA9063_REG_ADC_RES_L, .range_max = DA9063_REG_MON_A10_RES, }, { .range_min = DA9063_REG_SEQ, .range_max = DA9063_REG_SEQ, }, { .range_min = DA9063_REG_EN_32K, .range_max = DA9063_REG_EN_32K, }, { .range_min = DA9063_BB_REG_MON_REG_5, .range_max = DA9063_BB_REG_MON_REG_6, }, }; However this is a larger and more wide-ranging change compared to the one proposed by Marek, and would require other alterations to fit this in. Also I'm undecided to what it would really add apart from a new chip model: I have been told accessing the DA9063 RTC register locations has no effect in the DA9063L. If the maintainers are happy with this, and if a chip model addition is really needed in future it can be added later if required. Acked-by: Steve Twiss <stwiss.opensou...@diasemi.com> Regards, Steve > Signed-off-by: Marek Vasut <marek.vasut+rene...@gmail.com> > Cc: Geert Uytterhoeven <geert+rene...@glider.be> > Cc: Lee Jones <lee.jo...@linaro.org> > Cc: Mark Brown <broo...@kernel.org> > Cc: Steve Twiss <stwiss.opensou...@diasemi.com> > Cc: Wolfram Sang <wsa+rene...@sang-engineering.com> > Cc: linux-renesas-soc@vger.kernel.org > --- > drivers/mfd/da9063-i2c.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c index > 5544ce8e3363..84bbd2bbcd2a 100644 > --- a/drivers/mfd/da9063-i2c.c > +++ b/drivers/mfd/da9063-i2c.c > @@ -232,6 +232,7 @@ static struct regmap_config da9063_regmap_config = { > > static const struct of_device_id da9063_dt_ids[] = { > { .compatible = "dlg,da9063", }, > + { .comp