Re: [PATCH 4/7] dt-bindings: Add document for MT6397 MFD

Re: [PATCH 4/7] dt-bindings: Add document for MT6397 MFD

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 538 bytes --]

On Mon, Nov 17, 2014 at 03:40:24PM +0800, Flora Fu wrote:

> +		pmic {
> +			compatible = "mediatek,mt6397";
> +	}

This looks like it's missing a closing brace and is just generally weird
(having a subnode like this).

> +- regulators : The regulators of mt6397
> +	regulators {
> +		regulator constratints.
> +		refer Documentation/devicetree/bindings/regulator/regulator.txt
> +	};

You need to define the set of valid regulator names and how that maps
onto the physical device, look at other regulator binding documents for
examples.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 3/7] regulator: MT6397: Add support for MT6397 regulator

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2765 bytes --]

On Mon, Nov 17, 2014 at 03:40:23PM +0800, Flora Fu wrote:

This looks mostly good but there are a few fairly straightfoward things:

> @@ -725,5 +725,11 @@ config REGULATOR_WM8994
>  	  This driver provides support for the voltage regulators on the
>  	  WM8994 CODEC.
>  
> +config REGULATOR_MT6397
> +	tristate "MediaTek MT6397 PMIC"
> +	depends on MFD_MT6397
> +	help
> +	   This driver provides support for the voltage regulators on the MediaTek MT6397 PMIC.
> +
>  endif

Keep this and the Makefile sorted.

> +static int mt6397_buck_set_voltage_sel(struct regulator_dev *rdev, unsigned sel)
> +{


> +	vosel = info->buck_conf.vosel_reg;
> +	voselon = info->buck_conf.voselon_reg;
> +	vosel_mask = info->buck_conf.vosel_mask;

Please use the standard way of specifying data even if you can't use the
standard function.

> +
> +	ret = regmap_update_bits(rdev->regmap, vosel, vosel_mask, sel);
> +	if (ret != 0) {
> +		dev_err(&rdev->dev, "Failed to update vosel: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_update_bits(rdev->regmap, voselon, vosel_mask, sel);
> +	if (ret != 0) {
> +		dev_err(&rdev->dev, "Failed to update vosel_on: %d\n", ret);
> +		return ret;
> +	}
> +	return 0;

You should add comments here explaining what's going on - it's very
strange to have to write the same value to two different registers and
the names of the registers look suspicously like this is something to do
with a suspend mode...

Missing blank line before the return too.

> +static int mt6397_buck_get_voltage_sel(struct regulator_dev *rdev)
> +{

You could use the regmap based helper for this.

> +static int mt6397_ldo_set_voltage_sel(struct regulator_dev *rdev, unsigned sel)
> +{

The LDO operations appear to be identical to the standard regmap
helpers, please use them.

> +	if (is_fixed)
> +		return 0;

You should use the standard fixed voltage regulator support rather than 

> +static int mt6397_regulator_is_enabled(struct regulator_dev *rdev)
> +{

Again this looks like it should be using helpers.

> +#define MT6397_REGULATOR_OF_MATCH(_name, _id)			\
> +[MT6397_ID_##_id] = {						\
> +	.name = #_name,						\
> +	.driver_data = &mt6397_regulators[MT6397_ID_##_id],	\
> +}

Define regulators_node and of_match in the regulator desc and you can
remove both this table and all your DT matching code in the driver, the
core will handle it for you.

> +	if ((reg_value & 0xFF) == MT6397_REGULATOR_ID91) {
> +		j = MT6397_ID_VCAMIO;
> +		mt6397_regulator_matches[j].init_data->constraints.min_uV =
> +		1000000;
> +		mt6397_regulators[j].desc.volt_table = ldo_volt_table5_v2;
> +	}

Use a switch statement, that way other variants can be added more
easily.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/7] mfd: MT6397: Add regmap for MT8135 and MT6397 SoC

[Lee Jones]

On Mon, 17 Nov 2014, Flora Fu wrote:

> Add PMIC wrapper of MT8135 to access MFD MT6397.
> This is regmap of MT6397 MFD.
> 
> Signed-off-by: Flora Fu <flora.fu@mediatek.com>
> ---
>  drivers/mfd/Kconfig            |   8 +
>  drivers/mfd/Makefile           |   1 +
>  drivers/mfd/mt8135-pmic-wrap.c | 847 +++++++++++++++++++++++++++++++++++++++++

All of the PMIC functionality needs removing from MFD and placed
somewhere else.  I suggest either drivers/power or drivers/regulator.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/7] mfd: MT6397: Add regmap for MT8135 and MT6397 SoC

[Sascha Hauer]

On Tue, Nov 18, 2014 at 11:46:45AM +0000, Lee Jones wrote:
> On Mon, 17 Nov 2014, Flora Fu wrote:
> 
> > Add PMIC wrapper of MT8135 to access MFD MT6397.
> > This is regmap of MT6397 MFD.
> > 
> > Signed-off-by: Flora Fu <flora.fu@mediatek.com>
> > ---
> >  drivers/mfd/Kconfig            |   8 +
> >  drivers/mfd/Makefile           |   1 +
> >  drivers/mfd/mt8135-pmic-wrap.c | 847 +++++++++++++++++++++++++++++++++++++++++
> 
> All of the PMIC functionality needs removing from MFD and placed
> somewhere else.  I suggest either drivers/power or drivers/regulator.

This is no PMIC functionality. The MT8135 has a unit which is is used to
access the PMIC (which is not only a PMIC, but also Touchscreen
interface and other stuff). This unit is called pmic-wrapper in the
docs. See the introductory mail for a nice picture.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/7] mfd: MT6397: Add regmap for MT8135 and MT6397 SoC

[Lee Jones]

On Tue, 18 Nov 2014, Sascha Hauer wrote:

> On Tue, Nov 18, 2014 at 11:46:45AM +0000, Lee Jones wrote:
> > On Mon, 17 Nov 2014, Flora Fu wrote:
> > 
> > > Add PMIC wrapper of MT8135 to access MFD MT6397.
> > > This is regmap of MT6397 MFD.
> > > 
> > > Signed-off-by: Flora Fu <flora.fu@mediatek.com>
> > > ---
> > >  drivers/mfd/Kconfig            |   8 +
> > >  drivers/mfd/Makefile           |   1 +
> > >  drivers/mfd/mt8135-pmic-wrap.c | 847 +++++++++++++++++++++++++++++++++++++++++
> > 
> > All of the PMIC functionality needs removing from MFD and placed
> > somewhere else.  I suggest either drivers/power or drivers/regulator.
> 
> This is no PMIC functionality. The MT8135 has a unit which is is used to
> access the PMIC (which is not only a PMIC, but also Touchscreen
> interface and other stuff). This unit is called pmic-wrapper in the
> docs. See the introductory mail for a nice picture.

I saw the picture, it's very nice.  Whatever this is, it's not an
MFD.  It's a device which is located on an MFD.  There is far too much
functional (the operative word here) code contained in this patch.

I'm not sure where this device should live, but I'm keen for MFD not
to become a dumping ground for ill-fitting devices.  Happy for you to
create resources and register it from here, but the bulk for the
functional driver needs to live somewhere else. 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/7] mfd: MT6397: Add regmap for MT8135 and MT6397 SoC

[Sascha Hauer]

On Wed, Nov 19, 2014 at 05:04:54PM +0000, Lee Jones wrote:
> On Tue, 18 Nov 2014, Sascha Hauer wrote:
> 
> > On Tue, Nov 18, 2014 at 11:46:45AM +0000, Lee Jones wrote:
> > > On Mon, 17 Nov 2014, Flora Fu wrote:
> > > 
> > > > Add PMIC wrapper of MT8135 to access MFD MT6397.
> > > > This is regmap of MT6397 MFD.
> > > > 
> > > > Signed-off-by: Flora Fu <flora.fu@mediatek.com>
> > > > ---
> > > >  drivers/mfd/Kconfig            |   8 +
> > > >  drivers/mfd/Makefile           |   1 +
> > > >  drivers/mfd/mt8135-pmic-wrap.c | 847 +++++++++++++++++++++++++++++++++++++++++
> > > 
> > > All of the PMIC functionality needs removing from MFD and placed
> > > somewhere else.  I suggest either drivers/power or drivers/regulator.
> > 
> > This is no PMIC functionality. The MT8135 has a unit which is is used to
> > access the PMIC (which is not only a PMIC, but also Touchscreen
> > interface and other stuff). This unit is called pmic-wrapper in the
> > docs. See the introductory mail for a nice picture.
> 
> I saw the picture, it's very nice.  Whatever this is, it's not an
> MFD.  It's a device which is located on an MFD.  There is far too much
> functional (the operative word here) code contained in this patch.

The MT6397 is a classical MFD device. It has a PMIC, an audio amp and a
RTC. It is very tightly coupled to the SoC via SPI, but the SPI itself
is not directly visible on the SoC. It's accessible indirectly via the
PMIC-wrapper. Indeed the PMIC-wrapper is not MFD. Another dumping ground
that comes to my mind is drivers/soc/. We could move over there.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/7] mfd: MT6397: Add regmap for MT8135 and MT6397 SoC

[Lee Jones]

On Thu, 20 Nov 2014, Sascha Hauer wrote:

> On Wed, Nov 19, 2014 at 05:04:54PM +0000, Lee Jones wrote:
> > On Tue, 18 Nov 2014, Sascha Hauer wrote:
> > 
> > > On Tue, Nov 18, 2014 at 11:46:45AM +0000, Lee Jones wrote:
> > > > On Mon, 17 Nov 2014, Flora Fu wrote:
> > > > 
> > > > > Add PMIC wrapper of MT8135 to access MFD MT6397.
> > > > > This is regmap of MT6397 MFD.
> > > > > 
> > > > > Signed-off-by: Flora Fu <flora.fu@mediatek.com>
> > > > > ---
> > > > >  drivers/mfd/Kconfig            |   8 +
> > > > >  drivers/mfd/Makefile           |   1 +
> > > > >  drivers/mfd/mt8135-pmic-wrap.c | 847 +++++++++++++++++++++++++++++++++++++++++
> > > > 
> > > > All of the PMIC functionality needs removing from MFD and placed
> > > > somewhere else.  I suggest either drivers/power or drivers/regulator.
> > > 
> > > This is no PMIC functionality. The MT8135 has a unit which is is used to
> > > access the PMIC (which is not only a PMIC, but also Touchscreen
> > > interface and other stuff). This unit is called pmic-wrapper in the
> > > docs. See the introductory mail for a nice picture.
> > 
> > I saw the picture, it's very nice.  Whatever this is, it's not an
> > MFD.  It's a device which is located on an MFD.  There is far too much
> > functional (the operative word here) code contained in this patch.
> 
> The MT6397 is a classical MFD device. It has a PMIC, an audio amp and a
> RTC. It is very tightly coupled to the SoC via SPI, but the SPI itself
> is not directly visible on the SoC. It's accessible indirectly via the
> PMIC-wrapper. Indeed the PMIC-wrapper is not MFD. Another dumping ground
> that comes to my mind is drivers/soc/. We could move over there.

Thanks for understanding my point-of-view.  Hopefully there is a
suitable "dumping ground", or even better a proper place to move the
PMIC-wrapper to.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/7] regulator: MT6397: Add support for MT6397 regulator

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1602 bytes --]

On Fri, Nov 21, 2014 at 03:09:31PM +0800, Flora Fu wrote:
> On Mon, 2014-11-17 at 23:40 +0000, Mark Brown wrote:

> > > +	vosel = info->buck_conf.vosel_reg;
> > > +	voselon = info->buck_conf.voselon_reg;
> > > +	vosel_mask = info->buck_conf.vosel_mask;

> > Please use the standard way of specifying data even if you can't use the
> > standard function.

> Could you specify the standard way of specification data? Thanks. 

Using the fields in the regulator_desc as you can see from the standard
helpers.

> > You should add comments here explaining what's going on - it's very
> > strange to have to write the same value to two different registers and
> > the names of the registers look suspiciously like this is something to do
> > with a suspend mode...

> Yes, its is for suspend mode control usage.
> For registers "vosel", "voselon", they is called register mode or
> hardware control mode voltage settings. Register mode is a default mode
> on the buck control. For quickly normal/sleep mode switch, hardware
> control can be enabled by controlling buck output by a CTRL_PIN. In the
> following diagram, there is a static settings on vosel_sleep for suspend
> mode output. According to CTRL_PIN's level, Vout can have different
> output (voselon or vosel_sleep). 

You need to represent this in your driver, the sleep mode controls
should either be controlled using the suspend API or the GPIO control
needs to be visible in the driver.  It's also OK to ignore the GPIO
control for now and do it later if complex work is needed to represent
it in the driver.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]