All of lore.kernel.org
 help / color / mirror / Atom feed
From: AngeloGioacchino Del Regno  <angelogioacchino.delregno@collabora.com>
To: Mark Brown <broonie@kernel.org>
Cc: lgirdwood@gmail.com, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, matthias.bgg@gmail.com,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org
Subject: Re: [PATCH 2/4] regulator: Add driver for MT6331 PMIC regulators
Date: Mon, 23 May 2022 14:49:19 +0200	[thread overview]
Message-ID: <6cc68be9-e509-eae4-801d-997fdc01dcf2@collabora.com> (raw)
In-Reply-To: <YoepiTUfdhkYByo7@sirena.org.uk>

Il 20/05/22 16:45, Mark Brown ha scritto:
> On Fri, May 20, 2022 at 03:33:03PM +0200, AngeloGioacchino Del Regno wrote:
> 
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2022 Collabora Ltd.
>> + * Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> + *
> 
> Please make the entire comment a C++ one so things look more
> intentional.
> 

Will do.

>> +static const unsigned int ldo_volt_table10[] = {
>> +	1200000, 1300000, 1500000, 1800000,
>> +	1200000, 1300000, 1500000, 1800000,
>> +	1200000, 1300000, 1500000, 1800000,
>> +	1200000, 1300000, 1500000, 1800000,
>> +};
> 
> So the top bits of the voltate selection field just get ignored?  Might
> be easier to just write the code to not include the top bits.
> 

No, they're all valid values for real... but I guess that I can simplify
this voltage table by simply modifying the bitmask that we use for the
regulators that are using this table....

Example:

MT6331_LDO_S("ldo-vcamio", VCAM_IO,  ldo_volt_table10, MT6331_SYSLDO_CON3, 10,

	MT6331_SYSLDO_CON11, GENMASK(6, 3), MT6331_SYSLDO_CON3, GENMASK(1, 0),

	MT6331_EN_STATUS1, BIT(13)),

So for VCAM_IO it's CON11... we have VGP2, VGP3 using CON14, CON15, then there
are no more users for this table.
If I use GENMASK(4, 3), I can effectively leave only four voltage values in
ldo_volt_table10.

Also, I've just noticed a mistake on ldo-vmipi: that's supposed to have a mask
of GENMASK(5, 3), not (6, 3) - which is in turn also making me able to remove
the ldo_volt_table_3a.

Alright, will fix in the next one :))

>> +static int mt6331_get_status(struct regulator_dev *rdev)
>> +{
>> +	struct mt6331_regulator_info *info = rdev_get_drvdata(rdev);
>> +	u32 reg, en_mask, regval;
>> +	int ret;
>> +
>> +	if (info->qi > 0) {
>> +		reg = info->desc.enable_reg;
>> +		en_mask = info->qi;
> 
> If the regulator doesn't have status readback it shouldn't provide a
> get_status() operation.
> 

What I've understood is that when there's no "QI" flag, the enable register
will provide the regulator status (EN/DIS) acting like QI, that's why I've
added that if branch...

Anyway, I'll recheck this part before sending the next version!

>> +static int mt6331_ldo_set_mode(struct regulator_dev *rdev, unsigned int mode)
>> +{
>> +	int ret, val = 0;
>> +	struct mt6331_regulator_info *info = rdev_get_drvdata(rdev);
>> +
>> +	if (!info->modeset_mask) {
>> +		dev_err(&rdev->dev, "regulator %s doesn't support set_mode\n",
>> +			info->desc.name);
>> +		return -EINVAL;
>> +	}
> 
> Just don't provide the operation for these regulators then.  That'll
> mean a separate ops struct but that's fine.

Oki, I'll add new ops then.

Thanks for the review!
P.S.: I'll obviously apply the same suggestions on mt6332-regulator as well.

Cheers,
Angelo

WARNING: multiple messages have this Message-ID (diff)
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
To: Mark Brown <broonie@kernel.org>
Cc: lgirdwood@gmail.com, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, matthias.bgg@gmail.com,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org
Subject: Re: [PATCH 2/4] regulator: Add driver for MT6331 PMIC regulators
Date: Mon, 23 May 2022 14:49:19 +0200	[thread overview]
Message-ID: <6cc68be9-e509-eae4-801d-997fdc01dcf2@collabora.com> (raw)
In-Reply-To: <YoepiTUfdhkYByo7@sirena.org.uk>

Il 20/05/22 16:45, Mark Brown ha scritto:
> On Fri, May 20, 2022 at 03:33:03PM +0200, AngeloGioacchino Del Regno wrote:
> 
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2022 Collabora Ltd.
>> + * Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> + *
> 
> Please make the entire comment a C++ one so things look more
> intentional.
> 

Will do.

>> +static const unsigned int ldo_volt_table10[] = {
>> +	1200000, 1300000, 1500000, 1800000,
>> +	1200000, 1300000, 1500000, 1800000,
>> +	1200000, 1300000, 1500000, 1800000,
>> +	1200000, 1300000, 1500000, 1800000,
>> +};
> 
> So the top bits of the voltate selection field just get ignored?  Might
> be easier to just write the code to not include the top bits.
> 

No, they're all valid values for real... but I guess that I can simplify
this voltage table by simply modifying the bitmask that we use for the
regulators that are using this table....

Example:

MT6331_LDO_S("ldo-vcamio", VCAM_IO,  ldo_volt_table10, MT6331_SYSLDO_CON3, 10,

	MT6331_SYSLDO_CON11, GENMASK(6, 3), MT6331_SYSLDO_CON3, GENMASK(1, 0),

	MT6331_EN_STATUS1, BIT(13)),

So for VCAM_IO it's CON11... we have VGP2, VGP3 using CON14, CON15, then there
are no more users for this table.
If I use GENMASK(4, 3), I can effectively leave only four voltage values in
ldo_volt_table10.

Also, I've just noticed a mistake on ldo-vmipi: that's supposed to have a mask
of GENMASK(5, 3), not (6, 3) - which is in turn also making me able to remove
the ldo_volt_table_3a.

Alright, will fix in the next one :))

>> +static int mt6331_get_status(struct regulator_dev *rdev)
>> +{
>> +	struct mt6331_regulator_info *info = rdev_get_drvdata(rdev);
>> +	u32 reg, en_mask, regval;
>> +	int ret;
>> +
>> +	if (info->qi > 0) {
>> +		reg = info->desc.enable_reg;
>> +		en_mask = info->qi;
> 
> If the regulator doesn't have status readback it shouldn't provide a
> get_status() operation.
> 

What I've understood is that when there's no "QI" flag, the enable register
will provide the regulator status (EN/DIS) acting like QI, that's why I've
added that if branch...

Anyway, I'll recheck this part before sending the next version!

>> +static int mt6331_ldo_set_mode(struct regulator_dev *rdev, unsigned int mode)
>> +{
>> +	int ret, val = 0;
>> +	struct mt6331_regulator_info *info = rdev_get_drvdata(rdev);
>> +
>> +	if (!info->modeset_mask) {
>> +		dev_err(&rdev->dev, "regulator %s doesn't support set_mode\n",
>> +			info->desc.name);
>> +		return -EINVAL;
>> +	}
> 
> Just don't provide the operation for these regulators then.  That'll
> mean a separate ops struct but that's fine.

Oki, I'll add new ops then.

Thanks for the review!
P.S.: I'll obviously apply the same suggestions on mt6332-regulator as well.

Cheers,
Angelo

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

WARNING: multiple messages have this Message-ID (diff)
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
To: Mark Brown <broonie@kernel.org>
Cc: lgirdwood@gmail.com, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, matthias.bgg@gmail.com,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org
Subject: Re: [PATCH 2/4] regulator: Add driver for MT6331 PMIC regulators
Date: Mon, 23 May 2022 14:49:19 +0200	[thread overview]
Message-ID: <6cc68be9-e509-eae4-801d-997fdc01dcf2@collabora.com> (raw)
In-Reply-To: <YoepiTUfdhkYByo7@sirena.org.uk>

Il 20/05/22 16:45, Mark Brown ha scritto:
> On Fri, May 20, 2022 at 03:33:03PM +0200, AngeloGioacchino Del Regno wrote:
> 
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2022 Collabora Ltd.
>> + * Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> + *
> 
> Please make the entire comment a C++ one so things look more
> intentional.
> 

Will do.

>> +static const unsigned int ldo_volt_table10[] = {
>> +	1200000, 1300000, 1500000, 1800000,
>> +	1200000, 1300000, 1500000, 1800000,
>> +	1200000, 1300000, 1500000, 1800000,
>> +	1200000, 1300000, 1500000, 1800000,
>> +};
> 
> So the top bits of the voltate selection field just get ignored?  Might
> be easier to just write the code to not include the top bits.
> 

No, they're all valid values for real... but I guess that I can simplify
this voltage table by simply modifying the bitmask that we use for the
regulators that are using this table....

Example:

MT6331_LDO_S("ldo-vcamio", VCAM_IO,  ldo_volt_table10, MT6331_SYSLDO_CON3, 10,

	MT6331_SYSLDO_CON11, GENMASK(6, 3), MT6331_SYSLDO_CON3, GENMASK(1, 0),

	MT6331_EN_STATUS1, BIT(13)),

So for VCAM_IO it's CON11... we have VGP2, VGP3 using CON14, CON15, then there
are no more users for this table.
If I use GENMASK(4, 3), I can effectively leave only four voltage values in
ldo_volt_table10.

Also, I've just noticed a mistake on ldo-vmipi: that's supposed to have a mask
of GENMASK(5, 3), not (6, 3) - which is in turn also making me able to remove
the ldo_volt_table_3a.

Alright, will fix in the next one :))

>> +static int mt6331_get_status(struct regulator_dev *rdev)
>> +{
>> +	struct mt6331_regulator_info *info = rdev_get_drvdata(rdev);
>> +	u32 reg, en_mask, regval;
>> +	int ret;
>> +
>> +	if (info->qi > 0) {
>> +		reg = info->desc.enable_reg;
>> +		en_mask = info->qi;
> 
> If the regulator doesn't have status readback it shouldn't provide a
> get_status() operation.
> 

What I've understood is that when there's no "QI" flag, the enable register
will provide the regulator status (EN/DIS) acting like QI, that's why I've
added that if branch...

Anyway, I'll recheck this part before sending the next version!

>> +static int mt6331_ldo_set_mode(struct regulator_dev *rdev, unsigned int mode)
>> +{
>> +	int ret, val = 0;
>> +	struct mt6331_regulator_info *info = rdev_get_drvdata(rdev);
>> +
>> +	if (!info->modeset_mask) {
>> +		dev_err(&rdev->dev, "regulator %s doesn't support set_mode\n",
>> +			info->desc.name);
>> +		return -EINVAL;
>> +	}
> 
> Just don't provide the operation for these regulators then.  That'll
> mean a separate ops struct but that's fine.

Oki, I'll add new ops then.

Thanks for the review!
P.S.: I'll obviously apply the same suggestions on mt6332-regulator as well.

Cheers,
Angelo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2022-05-23 12:49 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-20 13:33 [PATCH 0/4] MediaTek Helio X10 MT6795 - MT6331/6332 Regulators AngeloGioacchino Del Regno
2022-05-20 13:33 ` AngeloGioacchino Del Regno
2022-05-20 13:33 ` AngeloGioacchino Del Regno
2022-05-20 13:33 ` [PATCH 1/4] dt-bindings: regulator: Add bindings for MT6331 regulator AngeloGioacchino Del Regno
2022-05-20 13:33   ` AngeloGioacchino Del Regno
2022-05-20 13:33   ` AngeloGioacchino Del Regno
2022-05-21 14:40   ` Krzysztof Kozlowski
2022-05-21 14:40     ` Krzysztof Kozlowski
2022-05-21 14:40     ` Krzysztof Kozlowski
2022-05-20 13:33 ` [PATCH 2/4] regulator: Add driver for MT6331 PMIC regulators AngeloGioacchino Del Regno
2022-05-20 13:33   ` AngeloGioacchino Del Regno
2022-05-20 13:33   ` AngeloGioacchino Del Regno
2022-05-20 14:45   ` Mark Brown
2022-05-20 14:45     ` Mark Brown
2022-05-20 14:45     ` Mark Brown
2022-05-23 12:49     ` AngeloGioacchino Del Regno [this message]
2022-05-23 12:49       ` AngeloGioacchino Del Regno
2022-05-23 12:49       ` AngeloGioacchino Del Regno
2022-05-23 13:00       ` Mark Brown
2022-05-23 13:00         ` Mark Brown
2022-05-23 13:00         ` Mark Brown
2022-05-23 13:22         ` AngeloGioacchino Del Regno
2022-05-23 13:22           ` AngeloGioacchino Del Regno
2022-05-23 13:22           ` AngeloGioacchino Del Regno
2022-05-23 13:43           ` Mark Brown
2022-05-23 13:43             ` Mark Brown
2022-05-23 13:43             ` Mark Brown
2022-05-23 13:53             ` AngeloGioacchino Del Regno
2022-05-23 13:53               ` AngeloGioacchino Del Regno
2022-05-23 13:53               ` AngeloGioacchino Del Regno
2022-05-20 13:33 ` [PATCH 3/4] dt-bindings: regulator: Add bindings for MT6332 regulator AngeloGioacchino Del Regno
2022-05-20 13:33   ` AngeloGioacchino Del Regno
2022-05-20 13:33   ` AngeloGioacchino Del Regno
2022-05-21 14:40   ` Krzysztof Kozlowski
2022-05-21 14:40     ` Krzysztof Kozlowski
2022-05-21 14:40     ` Krzysztof Kozlowski
2022-05-20 13:33 ` [PATCH 4/4] regulator: Add driver for MT6332 PMIC regulators AngeloGioacchino Del Regno
2022-05-20 13:33   ` AngeloGioacchino Del Regno
2022-05-20 13:33   ` AngeloGioacchino Del Regno

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6cc68be9-e509-eae4-801d-997fdc01dcf2@collabora.com \
    --to=angelogioacchino.delregno@collabora.com \
    --cc=broonie@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=krzysztof.kozlowski+dt@linaro.org \
    --cc=lgirdwood@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=matthias.bgg@gmail.com \
    --cc=robh+dt@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.