[PATCH 1/2] mfd: sec-core: Remove duplicated device type from sec_pmic

[PATCH 1/2] mfd: sec-core: Remove duplicated device type from sec_pmic

[Krzysztof Kozlowski]

The device type was stored in sec_pmic state container twice:
 - unsigned long type
 - int device_type

The 'type' field was never used outside of probe so it can be safely
removed.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/mfd/sec-core.c           | 5 +++--
 include/linux/mfd/samsung/core.h | 1 -
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mfd/sec-core.c b/drivers/mfd/sec-core.c
index d4682c6cbff5..fccfd71f8abd 100644
--- a/drivers/mfd/sec-core.c
+++ b/drivers/mfd/sec-core.c
@@ -251,6 +251,7 @@ static int sec_pmic_probe(struct i2c_client *i2c,
 	struct sec_platform_data *pdata = dev_get_platdata(&i2c->dev);
 	const struct regmap_config *regmap;
 	struct sec_pmic_dev *sec_pmic;
+	unsigned long device_type;
 	int ret;
 
 	sec_pmic = devm_kzalloc(&i2c->dev, sizeof(struct sec_pmic_dev),
@@ -262,7 +263,7 @@ static int sec_pmic_probe(struct i2c_client *i2c,
 	sec_pmic->dev = &i2c->dev;
 	sec_pmic->i2c = i2c;
 	sec_pmic->irq = i2c->irq;
-	sec_pmic->type = sec_i2c_get_driver_data(i2c, id);
+	device_type = sec_i2c_get_driver_data(i2c, id);
 
 	if (sec_pmic->dev->of_node) {
 		pdata = sec_pmic_i2c_parse_dt_pdata(sec_pmic->dev);
@@ -270,7 +271,7 @@ static int sec_pmic_probe(struct i2c_client *i2c,
 			ret = PTR_ERR(pdata);
 			return ret;
 		}
-		pdata->device_type = sec_pmic->type;
+		pdata->device_type = (int)device_type;
 	}
 	if (pdata) {
 		sec_pmic->device_type = pdata->device_type;
diff --git a/include/linux/mfd/samsung/core.h b/include/linux/mfd/samsung/core.h
index 84aaf6c25794..2633083262ea 100644
--- a/include/linux/mfd/samsung/core.h
+++ b/include/linux/mfd/samsung/core.h
@@ -49,7 +49,6 @@ struct sec_pmic_dev {
 	struct regmap_irq_chip_data *irq_data;
 
 	int ono;
-	unsigned long type;
 	bool wakeup;
 	bool wtsr_smpl;
 };
-- 
1.8.3.2

[PATCH 2/2] mfd: sec-core: Update sec_pmic documentation

[Krzysztof Kozlowski]

Update the documentation for sec_pmic state container structure to
reflect current code.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 include/linux/mfd/samsung/core.h | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/include/linux/mfd/samsung/core.h b/include/linux/mfd/samsung/core.h
index 2633083262ea..1923fe026c01 100644
--- a/include/linux/mfd/samsung/core.h
+++ b/include/linux/mfd/samsung/core.h
@@ -24,18 +24,23 @@ enum sec_device_type {
 };
 
 /**
- * struct sec_pmic_dev - s5m87xx master device for sub-drivers
- * @dev: master device of the chip (can be used to access platform data)
- * @pdata: pointer to private data used to pass platform data to child
- * @i2c: i2c client private data for regulator
- * @iolock: mutex for serializing io access
- * @irqlock: mutex for buslock
- * @irq_base: base IRQ number for sec-pmic, required for IRQs
- * @irq: generic IRQ number for s5m87xx
- * @ono: power onoff IRQ number for s5m87xx
- * @irq_masks_cur: currently active value
- * @irq_masks_cache: cached hardware value
- * @type: indicate which s5m87xx "variant" is used
+ * struct sec_pmic_dev - s2m/s5m master device for sub-drivers
+ * @dev:		Master device of the chip (can be used to access
+ *			platform data)
+ * @pdata:		Platform data from DTS or board files
+ * @regmap_pmic:	Regmap associated with PMIC's I2C address
+ * @i2c:		I2C client of the main driver
+ * @device_type:	Type of device, matches enum sec_device_type
+ * @irq_base:		Base IRQ number for device, required for IRQs
+ * @irq:		Generic IRQ number for device
+ * @irq_data:		Runtime data structure for IRQ controller
+ * @ono:		Power onoff IRQ number for s5m87xx
+ * @wakeup:		Whether or not this is a wakeup device
+ * @wtsr_smpl:		Whether or not to enable in RTC driver the Watchdog
+ *			Timer Software Reset (registers set to default value
+ *			after PWRHOLD falling) and Sudden Momentary Power Loss
+ *			(PMIC will enter power on sequence after short drop in
+ *			VBATT voltage).
  */
 struct sec_pmic_dev {
 	struct device *dev;
-- 
1.8.3.2

Re: [PATCH 1/2] mfd: sec-core: Remove duplicated device type from sec_pmic

[Lee Jones]

> The device type was stored in sec_pmic state container twice:
>  - unsigned long type
>  - int device_type

Can you explain that one of them is in the main container and the
other is stored in pdata, which in turn is located in the main
container, as this implies that both of them are located in the same
place, which is not quite true.

> The 'type' field was never used outside of probe so it can be safely
> removed.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---
>  drivers/mfd/sec-core.c           | 5 +++--
>  include/linux/mfd/samsung/core.h | 1 -
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mfd/sec-core.c b/drivers/mfd/sec-core.c
> index d4682c6cbff5..fccfd71f8abd 100644
> --- a/drivers/mfd/sec-core.c
> +++ b/drivers/mfd/sec-core.c
> @@ -251,6 +251,7 @@ static int sec_pmic_probe(struct i2c_client *i2c,
>  	struct sec_platform_data *pdata = dev_get_platdata(&i2c->dev);
>  	const struct regmap_config *regmap;
>  	struct sec_pmic_dev *sec_pmic;
> +	unsigned long device_type;
>  	int ret;
>  
>  	sec_pmic = devm_kzalloc(&i2c->dev, sizeof(struct sec_pmic_dev),
> @@ -262,7 +263,7 @@ static int sec_pmic_probe(struct i2c_client *i2c,
>  	sec_pmic->dev = &i2c->dev;
>  	sec_pmic->i2c = i2c;
>  	sec_pmic->irq = i2c->irq;
> -	sec_pmic->type = sec_i2c_get_driver_data(i2c, id);
> +	device_type = sec_i2c_get_driver_data(i2c, id);

Better to change the return type of 'sec_i2c_get_driver_data()' than
to rely on a cast later on.

>  	if (sec_pmic->dev->of_node) {
>  		pdata = sec_pmic_i2c_parse_dt_pdata(sec_pmic->dev);
> @@ -270,7 +271,7 @@ static int sec_pmic_probe(struct i2c_client *i2c,
>  			ret = PTR_ERR(pdata);
>  			return ret;
>  		}
> -		pdata->device_type = sec_pmic->type;
> +		pdata->device_type = (int)device_type;
>  	}
>  	if (pdata) {
>  		sec_pmic->device_type = pdata->device_type;
> diff --git a/include/linux/mfd/samsung/core.h b/include/linux/mfd/samsung/core.h
> index 84aaf6c25794..2633083262ea 100644
> --- a/include/linux/mfd/samsung/core.h
> +++ b/include/linux/mfd/samsung/core.h
> @@ -49,7 +49,6 @@ struct sec_pmic_dev {
>  	struct regmap_irq_chip_data *irq_data;
>  
>  	int ono;
> -	unsigned long type;
>  	bool wakeup;
>  	bool wtsr_smpl;
>  };
> -- 
> 1.8.3.2
> 

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

Re: [PATCH 2/2] mfd: sec-core: Update sec_pmic documentation

[Lee Jones]

> Update the documentation for sec_pmic state container structure to
> reflect current code.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---
>  include/linux/mfd/samsung/core.h | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/mfd/samsung/core.h b/include/linux/mfd/samsung/core.h
> index 2633083262ea..1923fe026c01 100644
> --- a/include/linux/mfd/samsung/core.h
> +++ b/include/linux/mfd/samsung/core.h
> @@ -24,18 +24,23 @@ enum sec_device_type {
>  };
>  
>  /**
> - * struct sec_pmic_dev - s5m87xx master device for sub-drivers
> - * @dev: master device of the chip (can be used to access platform data)
> - * @pdata: pointer to private data used to pass platform data to child
> - * @i2c: i2c client private data for regulator
> - * @iolock: mutex for serializing io access
> - * @irqlock: mutex for buslock
> - * @irq_base: base IRQ number for sec-pmic, required for IRQs
> - * @irq: generic IRQ number for s5m87xx
> - * @ono: power onoff IRQ number for s5m87xx
> - * @irq_masks_cur: currently active value
> - * @irq_masks_cache: cached hardware value
> - * @type: indicate which s5m87xx "variant" is used
> + * struct sec_pmic_dev - s2m/s5m master device for sub-drivers
> + * @dev:		Master device of the chip (can be used to access
> + *			platform data)

Please remove the part in ()'s.

a) There's no need for it - we know what's in 'struct device' and b)
if 'struct platform_data' is removed from 'struct device' this comment
will become incorrect. Easy maintainability and all that ...

> + * @pdata:		Platform data from DTS or board files

s/from/populated with data from

> + * @regmap_pmic:	Regmap associated with PMIC's I2C address
> + * @i2c:		I2C client of the main driver
> + * @device_type:	Type of device, matches enum sec_device_type
> + * @irq_base:		Base IRQ number for device, required for IRQs

Is there any reason for not using IRQ Domains?

> + * @irq:		Generic IRQ number for device
> + * @irq_data:		Runtime data structure for IRQ controller
> + * @ono:		Power onoff IRQ number for s5m87xx

Perhaps consider a better name for this in a future patch.

> + * @wakeup:		Whether or not this is a wakeup device
> + * @wtsr_smpl:		Whether or not to enable in RTC driver the Watchdog
> + *			Timer Software Reset (registers set to default value
> + *			after PWRHOLD falling) and Sudden Momentary Power Loss
> + *			(PMIC will enter power on sequence after short drop in
> + *			VBATT voltage).
>   */
>  struct sec_pmic_dev {
>  	struct device *dev;

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

Re: [PATCH 1/2] mfd: sec-core: Remove duplicated device type from sec_pmic

[Krzysztof Kozlowski]

On czw, 2014-04-17 at 09:26 +0100, Lee Jones wrote:
> > The device type was stored in sec_pmic state container twice:
> >  - unsigned long type
> >  - int device_type
> 
> Can you explain that one of them is in the main container and the
> other is stored in pdata, which in turn is located in the main
> container, as this implies that both of them are located in the same
> place, which is not quite true.

Sure, I'll expand this section. However they were both stored in the
same place - struct sec_pmic. Additionally device_type is stored in
pdata.

> 
> > The 'type' field was never used outside of probe so it can be safely
> > removed.
> > 
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > ---
> >  drivers/mfd/sec-core.c           | 5 +++--
> >  include/linux/mfd/samsung/core.h | 1 -
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/mfd/sec-core.c b/drivers/mfd/sec-core.c
> > index d4682c6cbff5..fccfd71f8abd 100644
> > --- a/drivers/mfd/sec-core.c
> > +++ b/drivers/mfd/sec-core.c
> > @@ -251,6 +251,7 @@ static int sec_pmic_probe(struct i2c_client *i2c,
> >  	struct sec_platform_data *pdata = dev_get_platdata(&i2c->dev);
> >  	const struct regmap_config *regmap;
> >  	struct sec_pmic_dev *sec_pmic;
> > +	unsigned long device_type;
> >  	int ret;
> >  
> >  	sec_pmic = devm_kzalloc(&i2c->dev, sizeof(struct sec_pmic_dev),
> > @@ -262,7 +263,7 @@ static int sec_pmic_probe(struct i2c_client *i2c,
> >  	sec_pmic->dev = &i2c->dev;
> >  	sec_pmic->i2c = i2c;
> >  	sec_pmic->irq = i2c->irq;
> > -	sec_pmic->type = sec_i2c_get_driver_data(i2c, id);
> > +	device_type = sec_i2c_get_driver_data(i2c, id);
> 
> Better to change the return type of 'sec_i2c_get_driver_data()' than
> to rely on a cast later on.

Hmmm... it was like that before Pankaj Dubey change (8f695de515b9).
Solving that issue (-Wpointer-to-int-cast) along with keeping 'int' as
return type of sec_i2c_get_driver_data would lead to one of:
1. Using temporary variable in ec_i2c_get_driver_data just for cast.

static inline int sec_i2c_get_driver_data(struct i2c_client *i2c,
						const struct i2c_device_id *id)
{
	unsigned long type;

	if (i2c->dev.of_node) {
		const struct of_device_id *match;
		match = of_match_node(sec_dt_match, i2c->dev.of_node);
		type = (unsigned long)match->data;
	}
	type = id->driver_data;

	return (int)type;
}

or
2. Double cast: (int)(unsigned long)match->data;

For me both are kind a ugly... What do you think?


> 
> >  	if (sec_pmic->dev->of_node) {
> >  		pdata = sec_pmic_i2c_parse_dt_pdata(sec_pmic->dev);
> > @@ -270,7 +271,7 @@ static int sec_pmic_probe(struct i2c_client *i2c,
> >  			ret = PTR_ERR(pdata);
> >  			return ret;
> >  		}
> > -		pdata->device_type = sec_pmic->type;
> > +		pdata->device_type = (int)device_type;
> >  	}
> >  	if (pdata) {
> >  		sec_pmic->device_type = pdata->device_type;
> > diff --git a/include/linux/mfd/samsung/core.h b/include/linux/mfd/samsung/core.h
> > index 84aaf6c25794..2633083262ea 100644
> > --- a/include/linux/mfd/samsung/core.h
> > +++ b/include/linux/mfd/samsung/core.h
> > @@ -49,7 +49,6 @@ struct sec_pmic_dev {
> >  	struct regmap_irq_chip_data *irq_data;
> >  
> >  	int ono;
> > -	unsigned long type;
> >  	bool wakeup;
> >  	bool wtsr_smpl;
> >  };
> > -- 
> > 1.8.3.2
> > 
> 

Re: [PATCH 2/2] mfd: sec-core: Update sec_pmic documentation

[Krzysztof Kozlowski]

On czw, 2014-04-17 at 09:32 +0100, Lee Jones wrote:
> > Update the documentation for sec_pmic state container structure to
> > reflect current code.
> > 
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > ---
> >  include/linux/mfd/samsung/core.h | 29 +++++++++++++++++------------
> >  1 file changed, 17 insertions(+), 12 deletions(-)
> > 
> > diff --git a/include/linux/mfd/samsung/core.h b/include/linux/mfd/samsung/core.h
> > index 2633083262ea..1923fe026c01 100644
> > --- a/include/linux/mfd/samsung/core.h
> > +++ b/include/linux/mfd/samsung/core.h
> > @@ -24,18 +24,23 @@ enum sec_device_type {
> >  };
> >  
> >  /**
> > - * struct sec_pmic_dev - s5m87xx master device for sub-drivers
> > - * @dev: master device of the chip (can be used to access platform data)
> > - * @pdata: pointer to private data used to pass platform data to child
> > - * @i2c: i2c client private data for regulator
> > - * @iolock: mutex for serializing io access
> > - * @irqlock: mutex for buslock
> > - * @irq_base: base IRQ number for sec-pmic, required for IRQs
> > - * @irq: generic IRQ number for s5m87xx
> > - * @ono: power onoff IRQ number for s5m87xx
> > - * @irq_masks_cur: currently active value
> > - * @irq_masks_cache: cached hardware value
> > - * @type: indicate which s5m87xx "variant" is used
> > + * struct sec_pmic_dev - s2m/s5m master device for sub-drivers
> > + * @dev:		Master device of the chip (can be used to access
> > + *			platform data)
> 
> Please remove the part in ()'s.

OK

> 
> a) There's no need for it - we know what's in 'struct device' and b)
> if 'struct platform_data' is removed from 'struct device' this comment
> will become incorrect. Easy maintainability and all that ...
> 
> > + * @pdata:		Platform data from DTS or board files
> 
> s/from/populated with data from

OK

> 
> > + * @regmap_pmic:	Regmap associated with PMIC's I2C address
> > + * @i2c:		I2C client of the main driver
> > + * @device_type:	Type of device, matches enum sec_device_type
> > + * @irq_base:		Base IRQ number for device, required for IRQs
> 
> Is there any reason for not using IRQ Domains?

No, I have it in my TODO list.

> 
> > + * @irq:		Generic IRQ number for device
> > + * @irq_data:		Runtime data structure for IRQ controller
> > + * @ono:		Power onoff IRQ number for s5m87xx
> 
> Perhaps consider a better name for this in a future patch.

Actually I couldn't find usage of it so I plan to do some cleanup and
get rid of it.

Thanks for review.

Best regards,
Krzysztof

> 
> > + * @wakeup:		Whether or not this is a wakeup device
> > + * @wtsr_smpl:		Whether or not to enable in RTC driver the Watchdog
> > + *			Timer Software Reset (registers set to default value
> > + *			after PWRHOLD falling) and Sudden Momentary Power Loss
> > + *			(PMIC will enter power on sequence after short drop in
> > + *			VBATT voltage).
> >   */
> >  struct sec_pmic_dev {
> >  	struct device *dev;
> 

Re: [PATCH 1/2] mfd: sec-core: Remove duplicated device type from sec_pmic

[Lee Jones]

> > > +	unsigned long device_type;
> > >  	int ret;
> > >  
> > >  	sec_pmic = devm_kzalloc(&i2c->dev, sizeof(struct sec_pmic_dev),
> > > @@ -262,7 +263,7 @@ static int sec_pmic_probe(struct i2c_client *i2c,
> > >  	sec_pmic->dev = &i2c->dev;
> > >  	sec_pmic->i2c = i2c;
> > >  	sec_pmic->irq = i2c->irq;
> > > -	sec_pmic->type = sec_i2c_get_driver_data(i2c, id);
> > > +	device_type = sec_i2c_get_driver_data(i2c, id);
> > 
> > Better to change the return type of 'sec_i2c_get_driver_data()' than
> > to rely on a cast later on.
> 
> Hmmm... it was like that before Pankaj Dubey change (8f695de515b9).
> Solving that issue (-Wpointer-to-int-cast) along with keeping 'int' as
> return type of sec_i2c_get_driver_data would lead to one of:
> 1. Using temporary variable in ec_i2c_get_driver_data just for cast.
> 
> static inline int sec_i2c_get_driver_data(struct i2c_client *i2c,
> 						const struct i2c_device_id *id)
> {
> 	unsigned long type;
> 
> 	if (i2c->dev.of_node) {
> 		const struct of_device_id *match;
> 		match = of_match_node(sec_dt_match, i2c->dev.of_node);
> 		type = (unsigned long)match->data;
> 	}
> 	type = id->driver_data;
> 
> 	return (int)type;
> }
> 
> or
> 2. Double cast: (int)(unsigned long)match->data;
> 
> For me both are kind a ugly... What do you think?

I'm saying, why don't you change everything; the return value of
sec_i2c_get_driver_data(), pdata->device_type and device_type to long
or unsigned long? Then the only cast you need to do is from void*.

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

Re: [PATCH 1/2] mfd: sec-core: Remove duplicated device type from sec_pmic

[Krzysztof Kozlowski]

On wto, 2014-04-22 at 09:09 +0100, Lee Jones wrote:
> > > > +	unsigned long device_type;
> > > >  	int ret;
> > > >  
> > > >  	sec_pmic = devm_kzalloc(&i2c->dev, sizeof(struct sec_pmic_dev),
> > > > @@ -262,7 +263,7 @@ static int sec_pmic_probe(struct i2c_client *i2c,
> > > >  	sec_pmic->dev = &i2c->dev;
> > > >  	sec_pmic->i2c = i2c;
> > > >  	sec_pmic->irq = i2c->irq;
> > > > -	sec_pmic->type = sec_i2c_get_driver_data(i2c, id);
> > > > +	device_type = sec_i2c_get_driver_data(i2c, id);
> > > 
> > > Better to change the return type of 'sec_i2c_get_driver_data()' than
> > > to rely on a cast later on.
> > 
> > Hmmm... it was like that before Pankaj Dubey change (8f695de515b9).
> > Solving that issue (-Wpointer-to-int-cast) along with keeping 'int' as
> > return type of sec_i2c_get_driver_data would lead to one of:
> > 1. Using temporary variable in ec_i2c_get_driver_data just for cast.
> > 
> > static inline int sec_i2c_get_driver_data(struct i2c_client *i2c,
> > 						const struct i2c_device_id *id)
> > {
> > 	unsigned long type;
> > 
> > 	if (i2c->dev.of_node) {
> > 		const struct of_device_id *match;
> > 		match = of_match_node(sec_dt_match, i2c->dev.of_node);
> > 		type = (unsigned long)match->data;
> > 	}
> > 	type = id->driver_data;
> > 
> > 	return (int)type;
> > }
> > 
> > or
> > 2. Double cast: (int)(unsigned long)match->data;
> > 
> > For me both are kind a ugly... What do you think?
> 
> I'm saying, why don't you change everything; the return value of
> sec_i2c_get_driver_data(), pdata->device_type and device_type to long
> or unsigned long? Then the only cast you need to do is from void*.

Aaaa, I got it. I'll sent next version of patch.

Best regards,
Krzysztof