pca953x: support working w/o platform data

pca953x: support working w/o platform data

[Christian Gmeiner]

Provide defaults for pca953x, so the driver can be used w/o
providing platform data.

Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
---
 pca953x.c |   21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)
diff -Nur linux-3.0-rc5/drivers/gpio//pca953x.c
linux-3.0-rc5__patched/drivers/gpio//pca953x.c
--- linux-3.0-rc5/drivers/gpio//pca953x.c	2011-06-28 04:12:22.000000000 +0200
+++ linux-3.0-rc5__patched/drivers/gpio//pca953x.c	2011-07-01
13:19:08.369130336 +0200
@@ -452,7 +452,7 @@
 	struct pca953x_platform_data *pdata = client->dev.platform_data;
 	int ret, offset = 0;

-	if (pdata->irq_base != -1
+	if (pdata && pdata->irq_base != -1
 			&& (id->driver_data & PCA_INT)) {
 		int lvl;

@@ -524,7 +524,7 @@
 	struct i2c_client *client = chip->client;
 	struct pca953x_platform_data *pdata = client->dev.platform_data;

-	if (pdata->irq_base != -1 && (id->driver_data & PCA_INT))
+	if (pdata && pdata->irq_base != -1 && (id->driver_data & PCA_INT))
 		dev_warn(&client->dev, "interrupt support not compiled in\n");

 	return 0;
@@ -643,6 +643,7 @@
 	struct pca953x_platform_data *pdata;
 	struct pca953x_chip *chip;
 	int ret = 0;
+	int invert = 0;

 	chip = kzalloc(sizeof(struct pca953x_chip), GFP_KERNEL);
 	if (chip == NULL)
@@ -660,15 +661,13 @@

 	if (pdata == NULL) {
 		dev_dbg(&client->dev, "no platform data\n");
-		ret = -EINVAL;
-		goto out_failed;
 	}

 	chip->client = client;

-	chip->gpio_start = pdata->gpio_base;
+	chip->gpio_start = pdata ? pdata->gpio_base : -1;

-	chip->names = pdata->names;
+	chip->names = pdata ? pdata->names : NULL;
 	chip->chip_type = id->driver_data & (PCA953X_TYPE | PCA957X_TYPE);

 	mutex_init(&chip->i2c_lock);
@@ -678,10 +677,12 @@
 	 */
 	pca953x_setup_gpio(chip, id->driver_data & PCA_GPIO_MASK);

+	invert = pdata ? pdata->invert : 0;
+
 	if (chip->chip_type == PCA953X_TYPE)
-		device_pca953x_init(chip, pdata->invert);
+		device_pca953x_init(chip, invert);
 	else if (chip->chip_type == PCA957X_TYPE)
-		device_pca957x_init(chip, pdata->invert);
+		device_pca957x_init(chip, invert);
 	else
 		goto out_failed;

@@ -693,7 +694,7 @@
 	if (ret)
 		goto out_failed_irq;

-	if (pdata->setup) {
+	if (pdata && pdata->setup) {
 		ret = pdata->setup(client, chip->gpio_chip.base,
 				chip->gpio_chip.ngpio, pdata->context);
 		if (ret < 0)
@@ -717,7 +718,7 @@
 	struct pca953x_chip *chip = i2c_get_clientdata(client);
 	int ret = 0;

-	if (pdata->teardown) {
+	if (pdata && pdata->teardown) {
 		ret = pdata->teardown(client, chip->gpio_chip.base,
 				chip->gpio_chip.ngpio, pdata->context);
 		if (ret < 0) {
---

Re: pca953x: support working w/o platform data

[Igor Grinberg]

On 07/01/11 14:33, Christian Gmeiner wrote:

> Provide defaults for pca953x, so the driver can be used w/o
> providing platform data.

Wouldn't it be better to provide a default pdata structure inside the driver
and use it in case no pdata supplied, so you will not have to patch the
driver all around checking each time if pdata is valid?

> Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
> ---
>  pca953x.c |   21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> diff -Nur linux-3.0-rc5/drivers/gpio//pca953x.c
> linux-3.0-rc5__patched/drivers/gpio//pca953x.c
> --- linux-3.0-rc5/drivers/gpio//pca953x.c	2011-06-28 04:12:22.000000000 +0200
> +++ linux-3.0-rc5__patched/drivers/gpio//pca953x.c	2011-07-01
> 13:19:08.369130336 +0200
> @@ -452,7 +452,7 @@
>  	struct pca953x_platform_data *pdata = client->dev.platform_data;
>  	int ret, offset = 0;
>
> -	if (pdata->irq_base != -1
> +	if (pdata && pdata->irq_base != -1
>  			&& (id->driver_data & PCA_INT)) {
>  		int lvl;
>
> @@ -524,7 +524,7 @@
>  	struct i2c_client *client = chip->client;
>  	struct pca953x_platform_data *pdata = client->dev.platform_data;
>
> -	if (pdata->irq_base != -1 && (id->driver_data & PCA_INT))
> +	if (pdata && pdata->irq_base != -1 && (id->driver_data & PCA_INT))
>  		dev_warn(&client->dev, "interrupt support not compiled in\n");
>
>  	return 0;
> @@ -643,6 +643,7 @@
>  	struct pca953x_platform_data *pdata;
>  	struct pca953x_chip *chip;
>  	int ret = 0;
> +	int invert = 0;
>
>  	chip = kzalloc(sizeof(struct pca953x_chip), GFP_KERNEL);
>  	if (chip == NULL)
> @@ -660,15 +661,13 @@
>
>  	if (pdata == NULL) {
>  		dev_dbg(&client->dev, "no platform data\n");
> -		ret = -EINVAL;
> -		goto out_failed;
>  	}
>
>  	chip->client = client;
>
> -	chip->gpio_start = pdata->gpio_base;
> +	chip->gpio_start = pdata ? pdata->gpio_base : -1;
>
> -	chip->names = pdata->names;
> +	chip->names = pdata ? pdata->names : NULL;
>  	chip->chip_type = id->driver_data & (PCA953X_TYPE | PCA957X_TYPE);
>
>  	mutex_init(&chip->i2c_lock);
> @@ -678,10 +677,12 @@
>  	 */
>  	pca953x_setup_gpio(chip, id->driver_data & PCA_GPIO_MASK);
>
> +	invert = pdata ? pdata->invert : 0;
> +
>  	if (chip->chip_type == PCA953X_TYPE)
> -		device_pca953x_init(chip, pdata->invert);
> +		device_pca953x_init(chip, invert);
>  	else if (chip->chip_type == PCA957X_TYPE)
> -		device_pca957x_init(chip, pdata->invert);
> +		device_pca957x_init(chip, invert);
>  	else
>  		goto out_failed;
>
> @@ -693,7 +694,7 @@
>  	if (ret)
>  		goto out_failed_irq;
>
> -	if (pdata->setup) {
> +	if (pdata && pdata->setup) {
>  		ret = pdata->setup(client, chip->gpio_chip.base,
>  				chip->gpio_chip.ngpio, pdata->context);
>  		if (ret < 0)
> @@ -717,7 +718,7 @@
>  	struct pca953x_chip *chip = i2c_get_clientdata(client);
>  	int ret = 0;
>
> -	if (pdata->teardown) {
> +	if (pdata && pdata->teardown) {
>  		ret = pdata->teardown(client, chip->gpio_chip.base,
>  				chip->gpio_chip.ngpio, pdata->context);
>  		if (ret < 0) {
> ---
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

-- 
Regards,
Igor.

Re: pca953x: support working w/o platform data

[Grant Likely]

On Fri, Jul 01, 2011 at 08:17:44PM +0300, Igor Grinberg wrote:
> On 07/01/11 14:33, Christian Gmeiner wrote:
> 
> > Provide defaults for pca953x, so the driver can be used w/o
> > providing platform data.
> 
> Wouldn't it be better to provide a default pdata structure inside the driver
> and use it in case no pdata supplied, so you will not have to patch the
> driver all around checking each time if pdata is valid?

I would agree.  However, you will need to adjust the structure of the
driver somewhat to do so.  Once a device is registered, the data
pointed to by platform_device->dev.platform_data must be treated as
immutable by the driver, otherwise driver unbind/rebind can become
horribly broken.

I often solve this by keeping a full copy of the pdata structure in
the driver's private data structure, and always referencing the 'safe'
local copy instead of dereferencing dev->platform_data every time.

g.

> 
> > Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
> > ---
> >  pca953x.c |   21 +++++++++++----------
> >  1 file changed, 11 insertions(+), 10 deletions(-)
> > diff -Nur linux-3.0-rc5/drivers/gpio//pca953x.c
> > linux-3.0-rc5__patched/drivers/gpio//pca953x.c
> > --- linux-3.0-rc5/drivers/gpio//pca953x.c	2011-06-28 04:12:22.000000000 +0200
> > +++ linux-3.0-rc5__patched/drivers/gpio//pca953x.c	2011-07-01
> > 13:19:08.369130336 +0200
> > @@ -452,7 +452,7 @@
> >  	struct pca953x_platform_data *pdata = client->dev.platform_data;
> >  	int ret, offset = 0;
> >
> > -	if (pdata->irq_base != -1
> > +	if (pdata && pdata->irq_base != -1
> >  			&& (id->driver_data & PCA_INT)) {
> >  		int lvl;
> >
> > @@ -524,7 +524,7 @@
> >  	struct i2c_client *client = chip->client;
> >  	struct pca953x_platform_data *pdata = client->dev.platform_data;
> >
> > -	if (pdata->irq_base != -1 && (id->driver_data & PCA_INT))
> > +	if (pdata && pdata->irq_base != -1 && (id->driver_data & PCA_INT))
> >  		dev_warn(&client->dev, "interrupt support not compiled in\n");
> >
> >  	return 0;
> > @@ -643,6 +643,7 @@
> >  	struct pca953x_platform_data *pdata;
> >  	struct pca953x_chip *chip;
> >  	int ret = 0;
> > +	int invert = 0;
> >
> >  	chip = kzalloc(sizeof(struct pca953x_chip), GFP_KERNEL);
> >  	if (chip == NULL)
> > @@ -660,15 +661,13 @@
> >
> >  	if (pdata == NULL) {
> >  		dev_dbg(&client->dev, "no platform data\n");
> > -		ret = -EINVAL;
> > -		goto out_failed;
> >  	}
> >
> >  	chip->client = client;
> >
> > -	chip->gpio_start = pdata->gpio_base;
> > +	chip->gpio_start = pdata ? pdata->gpio_base : -1;
> >
> > -	chip->names = pdata->names;
> > +	chip->names = pdata ? pdata->names : NULL;
> >  	chip->chip_type = id->driver_data & (PCA953X_TYPE | PCA957X_TYPE);
> >
> >  	mutex_init(&chip->i2c_lock);
> > @@ -678,10 +677,12 @@
> >  	 */
> >  	pca953x_setup_gpio(chip, id->driver_data & PCA_GPIO_MASK);
> >
> > +	invert = pdata ? pdata->invert : 0;
> > +
> >  	if (chip->chip_type == PCA953X_TYPE)
> > -		device_pca953x_init(chip, pdata->invert);
> > +		device_pca953x_init(chip, invert);
> >  	else if (chip->chip_type == PCA957X_TYPE)
> > -		device_pca957x_init(chip, pdata->invert);
> > +		device_pca957x_init(chip, invert);
> >  	else
> >  		goto out_failed;
> >
> > @@ -693,7 +694,7 @@
> >  	if (ret)
> >  		goto out_failed_irq;
> >
> > -	if (pdata->setup) {
> > +	if (pdata && pdata->setup) {
> >  		ret = pdata->setup(client, chip->gpio_chip.base,
> >  				chip->gpio_chip.ngpio, pdata->context);
> >  		if (ret < 0)
> > @@ -717,7 +718,7 @@
> >  	struct pca953x_chip *chip = i2c_get_clientdata(client);
> >  	int ret = 0;
> >
> > -	if (pdata->teardown) {
> > +	if (pdata && pdata->teardown) {
> >  		ret = pdata->teardown(client, chip->gpio_chip.base,
> >  				chip->gpio_chip.ngpio, pdata->context);
> >  		if (ret < 0) {
> > ---
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> >
> 
> -- 
> Regards,
> Igor.
> 

Re: pca953x: support working w/o platform data

[Igor Grinberg]

On 07/04/11 09:43, Grant Likely wrote:

> On Fri, Jul 01, 2011 at 08:17:44PM +0300, Igor Grinberg wrote:
>> On 07/01/11 14:33, Christian Gmeiner wrote:
>>
>>> Provide defaults for pca953x, so the driver can be used w/o
>>> providing platform data.
>> Wouldn't it be better to provide a default pdata structure inside the driver
>> and use it in case no pdata supplied, so you will not have to patch the
>> driver all around checking each time if pdata is valid?
>
> I would agree.  However, you will need to adjust the structure of the
> driver somewhat to do so.  Once a device is registered, the data
> pointed to by platform_device->dev.platform_data must be treated as
> immutable by the driver, otherwise driver unbind/rebind can become
> horribly broken.

The driver already copies several field from pdata to the chip structure.
Also, chip->dyn_pdata can be used for this purpose, but instead of
allocating it dynamically, it just can be  a part of the chip structure.

> I often solve this by keeping a full copy of the pdata structure in
> the driver's private data structure, and always referencing the 'safe'
> local copy instead of dereferencing dev->platform_data every time.

If this is done and the chip structure will contain all the information
needed (either flat, or in some kind of dyn_pdata), then indeed there
will be no need to access the pdata anymore.
Also, there will be no need to allocate pdata in pca953x_get_alt_pdata()
function.
I think this would be the most clean and safe way.

-- 
Regards,
Igor.

Re: pca953x: support working w/o platform data

[Grant Likely]

On Mon, Jul 04, 2011 at 10:45:03AM +0300, Igor Grinberg wrote:
> On 07/04/11 09:43, Grant Likely wrote:
> 
> > On Fri, Jul 01, 2011 at 08:17:44PM +0300, Igor Grinberg wrote:
> >> On 07/01/11 14:33, Christian Gmeiner wrote:
> >>
> >>> Provide defaults for pca953x, so the driver can be used w/o
> >>> providing platform data.
> >> Wouldn't it be better to provide a default pdata structure inside the driver
> >> and use it in case no pdata supplied, so you will not have to patch the
> >> driver all around checking each time if pdata is valid?
> >
> > I would agree.  However, you will need to adjust the structure of the
> > driver somewhat to do so.  Once a device is registered, the data
> > pointed to by platform_device->dev.platform_data must be treated as
> > immutable by the driver, otherwise driver unbind/rebind can become
> > horribly broken.
> 
> The driver already copies several field from pdata to the chip structure.
> Also, chip->dyn_pdata can be used for this purpose, but instead of
> allocating it dynamically, it just can be  a part of the chip structure.
> 
> > I often solve this by keeping a full copy of the pdata structure in
> > the driver's private data structure, and always referencing the 'safe'
> > local copy instead of dereferencing dev->platform_data every time.
> 
> If this is done and the chip structure will contain all the information
> needed (either flat, or in some kind of dyn_pdata), then indeed there
> will be no need to access the pdata anymore.
> Also, there will be no need to allocate pdata in pca953x_get_alt_pdata()
> function.
> I think this would be the most clean and safe way.

Yup!  :-)

g.