Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1380597 > unrolled thread

Re: [PATCH v2 4/5] iio: health: afe4404: use regmap to retrieve struct device

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 4/5] iio: health: afe4404: use regmap to retrieve  struct device Jonathan Cameron <jic23@kernel.org> - 2016-04-16 21:30 +0200
    Re: [PATCH v2 4/5] iio: health: afe4404: use regmap to retrieve  struct device "Andrew F. Davis" <afd@ti.com> - 2016-04-17 20:10 +0200
      Re: [PATCH v2 4/5] iio: health: afe4404: use regmap to retrieve  struct device Alison Schofield <amsfield22@gmail.com> - 2016-04-18 07:00 +0200
        Re: [PATCH v2 4/5] iio: health: afe4404: use regmap to retrieve  struct device "Andrew F. Davis" <afd@ti.com> - 2016-04-18 18:00 +0200
          Re: [PATCH v2 4/5] iio: health: afe4404: use regmap to retrieve  struct device Jonathan Cameron <jic23@kernel.org> - 2016-04-18 21:30 +0200

#1380597 — Re: [PATCH v2 4/5] iio: health: afe4404: use regmap to retrieve struct device

On 10/04/16 20:07, Alison Schofield wrote:
> Driver includes struct regmap and struct device in its global data.
> Remove the struct device and use regmap API to retrieve device info.
> 
> Patch created using Coccinelle plus manual edits.
> 
> Signed-off-by: Alison Schofield <amsfield22@gmail.com>
Cc'd Andrew
> ---
>  drivers/iio/health/afe4404.c | 38 +++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/iio/health/afe4404.c b/drivers/iio/health/afe4404.c
> index 5096a46..9cd8590 100644
> --- a/drivers/iio/health/afe4404.c
> +++ b/drivers/iio/health/afe4404.c
> @@ -107,14 +107,12 @@
>  
>  /**
>   * struct afe4404_data
> - * @dev - Device structure
>   * @regmap - Register map of the device
>   * @regulator - Pointer to the regulator for the IC
>   * @trig - IIO trigger for this device
>   * @irq - ADC_RDY line interrupt number
>   */
>  struct afe4404_data {
> -	struct device *dev;
>  	struct regmap *regmap;
>  	struct regulator *regulator;
>  	struct iio_trigger *trig;
> @@ -534,54 +532,54 @@ static int afe4404_probe(struct i2c_client *client,
>  	afe = iio_priv(indio_dev);
>  	i2c_set_clientdata(client, indio_dev);
>  
> -	afe->dev = &client->dev;
>  	afe->irq = client->irq;
>  
>  	afe->regmap = devm_regmap_init_i2c(client, &afe4404_regmap_config);
>  	if (IS_ERR(afe->regmap)) {
> -		dev_err(afe->dev, "Unable to allocate register map\n");
> +		dev_err(&client->dev, "Unable to allocate register map\n");
>  		return PTR_ERR(afe->regmap);
>  	}
>  
> -	afe->regulator = devm_regulator_get(afe->dev, "tx_sup");
> +	afe->regulator = devm_regulator_get(&client->dev, "tx_sup");
>  	if (IS_ERR(afe->regulator)) {
> -		dev_err(afe->dev, "Unable to get regulator\n");
> +		dev_err(&client->dev, "Unable to get regulator\n");
>  		return PTR_ERR(afe->regulator);
>  	}
>  	ret = regulator_enable(afe->regulator);
>  	if (ret) {
> -		dev_err(afe->dev, "Unable to enable regulator\n");
> +		dev_err(&client->dev, "Unable to enable regulator\n");
>  		return ret;
>  	}
>  
>  	ret = regmap_write(afe->regmap, AFE440X_CONTROL0,
>  			   AFE440X_CONTROL0_SW_RESET);
>  	if (ret) {
> -		dev_err(afe->dev, "Unable to reset device\n");
> +		dev_err(&client->dev, "Unable to reset device\n");
>  		goto disable_reg;
>  	}
>  
>  	ret = regmap_multi_reg_write(afe->regmap, afe4404_reg_sequences,
>  				     ARRAY_SIZE(afe4404_reg_sequences));
>  	if (ret) {
> -		dev_err(afe->dev, "Unable to set register defaults\n");
> +		dev_err(&client->dev, "Unable to set register defaults\n");
>  		goto disable_reg;
>  	}
>  
>  	indio_dev->modes = INDIO_DIRECT_MODE;
> -	indio_dev->dev.parent = afe->dev;
> +	indio_dev->dev.parent = &client->dev;
>  	indio_dev->channels = afe4404_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(afe4404_channels);
>  	indio_dev->name = AFE4404_DRIVER_NAME;
>  	indio_dev->info = &afe4404_iio_info;
>  
>  	if (afe->irq > 0) {
> -		afe->trig = devm_iio_trigger_alloc(afe->dev,
> +		afe->trig = devm_iio_trigger_alloc(&client->dev,
>  						   "%s-dev%d",
>  						   indio_dev->name,
>  						   indio_dev->id);
>  		if (!afe->trig) {
> -			dev_err(afe->dev, "Unable to allocate IIO trigger\n");
> +			dev_err(&client->dev,
> +				"Unable to allocate IIO trigger\n");
>  			ret = -ENOMEM;
>  			goto disable_reg;
>  		}
> @@ -589,21 +587,22 @@ static int afe4404_probe(struct i2c_client *client,
>  		iio_trigger_set_drvdata(afe->trig, indio_dev);
>  
>  		afe->trig->ops = &afe4404_trigger_ops;
> -		afe->trig->dev.parent = afe->dev;
> +		afe->trig->dev.parent = &client->dev;
>  
>  		ret = iio_trigger_register(afe->trig);
>  		if (ret) {
> -			dev_err(afe->dev, "Unable to register IIO trigger\n");
> +			dev_err(&client->dev,
> +				"Unable to register IIO trigger\n");
>  			goto disable_reg;
>  		}
>  
> -		ret = devm_request_threaded_irq(afe->dev, afe->irq,
> +		ret = devm_request_threaded_irq(&client->dev, afe->irq,
>  						iio_trigger_generic_data_rdy_poll,
>  						NULL, IRQF_ONESHOT,
>  						AFE4404_DRIVER_NAME,
>  						afe->trig);
>  		if (ret) {
> -			dev_err(afe->dev, "Unable to request IRQ\n");
> +			dev_err(&client->dev, "Unable to request IRQ\n");
>  			goto disable_reg;
>  		}
>  	}
> @@ -611,13 +610,13 @@ static int afe4404_probe(struct i2c_client *client,
>  	ret = iio_triggered_buffer_setup(indio_dev, &iio_pollfunc_store_time,
>  					 afe4404_trigger_handler, NULL);
>  	if (ret) {
> -		dev_err(afe->dev, "Unable to setup buffer\n");
> +		dev_err(&client->dev, "Unable to setup buffer\n");
>  		goto unregister_trigger;
>  	}
>  
>  	ret = iio_device_register(indio_dev);
>  	if (ret) {
> -		dev_err(afe->dev, "Unable to register IIO device\n");
> +		dev_err(&client->dev, "Unable to register IIO device\n");
>  		goto unregister_triggered_buffer;
>  	}
>  
> @@ -638,6 +637,7 @@ static int afe4404_remove(struct i2c_client *client)
>  {
>  	struct iio_dev *indio_dev = i2c_get_clientdata(client);
>  	struct afe4404_data *afe = iio_priv(indio_dev);
> +	struct device *dev = regmap_get_device(afe->regmap);
>  	int ret;
>  
>  	iio_device_unregister(indio_dev);
> @@ -649,7 +649,7 @@ static int afe4404_remove(struct i2c_client *client)
>  
>  	ret = regulator_disable(afe->regulator);
>  	if (ret) {
> -		dev_err(afe->dev, "Unable to disable regulator\n");
> +		dev_err(dev, "Unable to disable regulator\n");
>  		return ret;
>  	}
>  
> 

[toc] | [next] | [standalone]

#1380817

On 04/16/2016 02:22 PM, Jonathan Cameron wrote:
> On 10/04/16 20:07, Alison Schofield wrote:
>> Driver includes struct regmap and struct device in its global data.
>> Remove the struct device and use regmap API to retrieve device info.
>>

Why? This adds nothing but more code to get dev through some
container_of trickery when we could just keep a dev pointer in the data
structure.

Andrew

[toc] | [prev] | [next] | [standalone]

#1381251

On Sun, Apr 17, 2016 at 01:07:52PM -0500, Andrew F. Davis wrote:
> On 04/16/2016 02:22 PM, Jonathan Cameron wrote:
> > On 10/04/16 20:07, Alison Schofield wrote:
> >> Driver includes struct regmap and struct device in its global data.
> >> Remove the struct device and use regmap API to retrieve device info.
> >>
> 
> Why? This adds nothing but more code to get dev through some
> container_of trickery when we could just keep a dev pointer in the data
> structure.
> 
> Andrew

Thanks for the review and response.  The why would be for
simplification and uniformity across IIO.

I think I see your point in general, but not sure I get your
specific concerns with these afe4403/04 drivers.

The drivers only use the device struct in probe and then
again at device remove time.  At probe, the change no
longer stores it in the global data. At remove the
regmap_get_device() func is a simple dereference to retrieve
the device struct. That's the simplification: we don't carry
that ptr in global data waiting for the opportunity to use it
at device remove.  We just find it when we need it at device
remove.  (Perhaps these devices are getting removed frequently?)

Regards,
alisons

[toc] | [prev] | [next] | [standalone]

#1381848

On 04/17/2016 11:56 PM, Alison Schofield wrote:
> On Sun, Apr 17, 2016 at 01:07:52PM -0500, Andrew F. Davis wrote:
>> On 04/16/2016 02:22 PM, Jonathan Cameron wrote:
>>> On 10/04/16 20:07, Alison Schofield wrote:
>>>> Driver includes struct regmap and struct device in its global data.
>>>> Remove the struct device and use regmap API to retrieve device info.
>>>>
>>
>> Why? This adds nothing but more code to get dev through some
>> container_of trickery when we could just keep a dev pointer in the data
>> structure.
>>
>> Andrew
> 
> Thanks for the review and response.  The why would be for
> simplification and uniformity across IIO.
> 

I'm all for simplification and uniformity but I think this will take us
in the wrong direction, a lot drivers do not use regmap for instance and
so will need another method to lookup the device struct.

> I think I see your point in general, but not sure I get your
> specific concerns with these afe4403/04 drivers.
> 
> The drivers only use the device struct in probe and then
> again at device remove time.  At probe, the change no
> longer stores it in the global data. At remove the
> regmap_get_device() func is a simple dereference to retrieve
> the device struct. That's the simplification: we don't carry
> that ptr in global data waiting for the opportunity to use it
> at device remove.  We just find it when we need it at device
> remove.

A lot of what drivers store in their per-instance data structure is only
held for use in remove. Using afe4404 as an example trig and irq are
also only stored for removal. A good driver framework will pass in
relevant information to the driver callbacks, and so instance data
structures have gotten very small, for afe4404 only regmap and regulator
are actually used during runtime currently.

The direction that would really be of best simplification would be to
remove the need for the remove function from these drivers, if
iio_trigger_register and iio_triggered_buffer_setup had devm_ versions
(like most other setup functions) everything would automatically be
cleaned up properly without needing a remove function. Then I would have
no problem removing the unused device structure pointer, but for now
their will never be a simpler trick to getting the device pointer than
simply storing it in the instance data.

> (Perhaps these devices are getting removed frequently?)
> 

Quite the opposite, same for many drivers, the remove path is
dangerously untested in the real world, even more reason it's nice to
not need remove and let devm_ cleanup for us :)

Thanks,
Andrew

[toc] | [prev] | [next] | [standalone]

#1381994

On 18/04/16 16:53, Andrew F. Davis wrote:
> On 04/17/2016 11:56 PM, Alison Schofield wrote:
>> On Sun, Apr 17, 2016 at 01:07:52PM -0500, Andrew F. Davis wrote:
>>> On 04/16/2016 02:22 PM, Jonathan Cameron wrote:
>>>> On 10/04/16 20:07, Alison Schofield wrote:
>>>>> Driver includes struct regmap and struct device in its global data.
>>>>> Remove the struct device and use regmap API to retrieve device info.
>>>>>
>>>
>>> Why? This adds nothing but more code to get dev through some
>>> container_of trickery when we could just keep a dev pointer in the data
>>> structure.
>>>
>>> Andrew
>>
>> Thanks for the review and response.  The why would be for
>> simplification and uniformity across IIO.
>>
> 
> I'm all for simplification and uniformity but I think this will take us
> in the wrong direction, a lot drivers do not use regmap for instance and
> so will need another method to lookup the device struct.
> 
>> I think I see your point in general, but not sure I get your
>> specific concerns with these afe4403/04 drivers.
>>
>> The drivers only use the device struct in probe and then
>> again at device remove time.  At probe, the change no
>> longer stores it in the global data. At remove the
>> regmap_get_device() func is a simple dereference to retrieve
>> the device struct. That's the simplification: we don't carry
>> that ptr in global data waiting for the opportunity to use it
>> at device remove.  We just find it when we need it at device
>> remove.
> 
> A lot of what drivers store in their per-instance data structure is only
> held for use in remove. Using afe4404 as an example trig and irq are
> also only stored for removal. A good driver framework will pass in
> relevant information to the driver callbacks, and so instance data
> structures have gotten very small, for afe4404 only regmap and regulator
> are actually used during runtime currently.
The difference here is that we are handing over the device access to
entirely be via regmap - so to my mind we should probably also hand
over the dev pointer itself (which in fact we are doing anyway) as such
it makes sense to retrieve it from regmap when needed.  Anyhow, whilst
I originally suggested this (in a driver review a while back) it's not
an important issue and can be left to the tastes of individual authors.

Alison, you win some you loose some with this sort of cleanup patch!
In this particular case the fact that it was used as a convenient
place to get hold of it in the probe function made the patch a lot
more 'noisy' than it would otherwise have been.
> 
> The direction that would really be of best simplification would be to
> remove the need for the remove function from these drivers, if
> iio_trigger_register and iio_triggered_buffer_setup had devm_ versions
> (like most other setup functions) everything would automatically be
> cleaned up properly without needing a remove function. Then I would have
> no problem removing the unused device structure pointer, but for now
> their will never be a simpler trick to getting the device pointer than
> simply storing it in the instance data.
Whilst that might be nice.... Actually most devices need to do something
very much device specific in their shutdown - such as turning themselves off
so devm still only takes us so far.  Still I don't mind patches proposing
moving more stuff over to devm - they do help for some devices
(though the devm_iio_device_register still causes me a enough issues in
incorrect usage that I'm doubtful that accepting that one was ever a good
idea!)
> 
>> (Perhaps these devices are getting removed frequently?)
>>
> 
> Quite the opposite, same for many drivers, the remove path is
> dangerously untested in the real world, even more reason it's nice to
> not need remove and let devm_ cleanup for us :)

> 
> Thanks,
> Andrew
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web