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

Groups > linux.kernel > #1721796 > unrolled thread

[PATCH v6 0/2] ZII RAVE platform driver

Back to article view | Back to linux.kernel

Contents

  [PATCH v6 0/2] ZII RAVE platform driver Andrey Smirnov <andrew.smirnov@gmail.com> - 2017-08-28 18:40 +0200
    Re: [PATCH v6 1/2] platform: Add driver for RAVE Supervisory  Processor Pavel Machek <pavel@ucw.cz> - 2017-08-30 13:00 +0200
      Re: [PATCH v6 1/2] platform: Add driver for RAVE Supervisory Processor Andrey Smirnov <andrew.smirnov@gmail.com> - 2017-08-30 21:10 +0200
        Re: [PATCH v6 1/2] platform: Add driver for RAVE Supervisory  Processor Pavel Machek <pavel@ucw.cz> - 2017-08-30 22:40 +0200
          Re: [PATCH v6 1/2] platform: Add driver for RAVE Supervisory  Processor Pavel Machek <pavel@ucw.cz> - 2017-08-31 10:20 +0200
            Re: [PATCH v6 1/2] platform: Add driver for RAVE Supervisory  Processor Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2017-08-31 11:10 +0200
              Re: [PATCH v6 1/2] platform: Add driver for RAVE Supervisory  Processor Pavel Machek <pavel@ucw.cz> - 2017-08-31 11:20 +0200
          Re: [PATCH v6 1/2] platform: Add driver for RAVE Supervisory  Processor Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2017-08-31 10:20 +0200
    Re: [PATCH v6 1/2] platform: Add driver for RAVE Supervisory  Processor Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-08-31 18:50 +0200

#1721796 — [PATCH v6 0/2] ZII RAVE platform driver

Hi everyone,

This patch series is v6 of the driver for supervisory processor found
on RAVE series of devices from ZII. Supervisory processor is a PIC
microcontroller connected to various electrical subsystems on RAVE
devices whose firmware implements protocol to command/qery them.

Changes since [v5]:

    - Fixed a build break, introduced by a last minute change in [v5]

    - Moved majority of attributes that were exposed over sysfs to debugfs

    - Document remaining sysfs attributes in Documentation/ABI/testing/sysfs-platform-rave-sp

Changes since [v4]:

    - Replaced usage of DEVICE_ATTR with DEVICE_ATTR_RW

    - Fixed a number of warnings produces by sparse tool

    - Incorporated event more feedback from Andy Shevchenko

    - Collected Reviewed-by from Andy

Changes since [v3]:

    - Re-collected lost Acked-by from Rob

    - Incorporated further feedback from Andy Shevchenko

    - Dropped useless change (stray newline) to drivers/mfd/Makefile

Changes since [v2]:

    - Fixed swapped command codes in rave_sp_common_get_boot_source()
      and rave_sp_common_set_boot_source() revealed by further testing
      of the code

    - Incorporated feedback from Andy Shevchenko

Changes since [v1]:

    - Updated wording in DT-bindings as per Rob's request.

    - Collected Rob's Acked-by for patch 2/2

NOTE:

 * The driver for "zii,rave-sp-watchdog" exists, but I haven't
   submitted it yet, becuase I wanted to make sure that API exposed by
   this MFD is acceptable and doesn't need drastic changes.

 * This driver is dependent on crc_ccitt_false() introduced in
   2da9378d531f8cc6670c7497f20d936b706ab80b in 'linux-next'

Feedback is greatly appreciated!

Thanks,
Andrey Smirnov

[v5] lkml.kernel.org/r/20170728142704.11156-1-andrew.smirnov@gmail.com
[v4] lkml.kernel.org/r/20170725184450.13171-1-andrew.smirnov@gmail.com
[v3] lkml.kernel.org/r/20170724150915.4824-1-andrew.smirnov@gmail.com
[v2] lkml.kernel.org/r/20170718175604.11735-1-andrew.smirnov@gmail.com
[v1] lkml.kernel.org/r/20170710170449.4544-1-andrew.smirnov@gmail.com

Andrey Smirnov (2):
  platform: Add driver for RAVE Supervisory Processor
  dt-bindings: mfd: Add bindings for ZII RAVE devices

 Documentation/ABI/testing/sysfs-platform-rave-sp   |   35 +
 .../devicetree/bindings/mfd/zii,rave-sp.txt        |   39 +
 drivers/platform/Kconfig                           |    2 +
 drivers/platform/Makefile                          |    1 +
 drivers/platform/rave/Kconfig                      |   26 +
 drivers/platform/rave/Makefile                     |    1 +
 drivers/platform/rave/rave-sp.c                    | 1190 ++++++++++++++++++++
 include/linux/rave-sp.h                            |   54 +
 8 files changed, 1348 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-rave-sp
 create mode 100644 Documentation/devicetree/bindings/mfd/zii,rave-sp.txt
 create mode 100644 drivers/platform/rave/Kconfig
 create mode 100644 drivers/platform/rave/Makefile
 create mode 100644 drivers/platform/rave/rave-sp.c
 create mode 100644 include/linux/rave-sp.h

-- 
2.13.5

[toc] | [next] | [standalone]

#1723200 — Re: [PATCH v6 1/2] platform: Add driver for RAVE Supervisory Processor

[Multipart message — attachments visible in raw view] — view raw

Hi!

> Add a driver for RAVE Supervisory Processor, an MCU implementing
> varoius bits of housekeeping functionality (watchdoging, backlight
> control, LED control, etc) on RAVE family of products by Zodiac
> Inflight Innovations.
> 
> This driver implementes core MFD/serdev device as well as
> communication subroutines necessary for commanding the device.

> diff --git a/Documentation/ABI/testing/sysfs-platform-rave-sp b/Documentation/ABI/testing/sysfs-platform-rave-sp
> new file mode 100644
> index 000000000000..81bdc54ba857
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-platform-rave-sp
> @@ -0,0 +1,35 @@
> +What:		/sys/devices/platform/*/serial*/serial*-*/reset_reason
> +Date:		Aug 2017
> +Contact:	"Andrey Smirnov" <andrew.smirnov@gmail.com>
> +Description:    (RO) Indicates reason for last reset.
> +
> +		The following values can be reported:
> +		    * 0  -> Normal Power Off
> +		    * 1  -> Hardware Watchdog
> +		    * 2  -> Software Watchdog
> +		    * 3  -> Input Voltage Out Of Range
> +		    * 4  -> Host Requested
> +		    * 5  -> Temperature Out Of Range
> +		    * 6  -> User Requested (via long Power Button press)
> +		    * 7  -> Illegal Configuration Word
> +		    * 8  -> Illegal Insturction

Typo here.

> +		    * 9  -> Illegal Trap
> +		    * 10 -> Unknown
> +		    * 11 -> Crew Panel Requested

Anyway... If you move management chip to .. I don't know, i2c, the
path would change. Also it would be different path on N900. Userland
should not have to deal with this.

And... this should really be string, as the list will need to grow on
different hardware.

Plus we'll really need better explanations. What is difference between
"normal power off" and "host requested"? 

> +What:		/sys/devices/platform/*/serial*/serial*-*/boot_source
> +Date:		Aug 2017
> +Contact:	"Andrey Smirnov" <andrew.smirnov@gmail.com>
> +Description:    (RW) Indicates currently selected boot source.
> +
> +		The following values are valid:
> +		    * 0  -> SD card
> +		    * 1  -> eMMC
> +		    * 2  -> SPI NOR
> +
> +		NOTE: Setting boot source on RDU1 hardware is
> +		currently not implemented

Same comments apply here.

> +RAVE_SP_DEFINE_SEQ_FILE_PRINT_FUNCTION(part_number_firmware);
> +RAVE_SP_DEFINE_SEQ_FILE_PRINT_FUNCTION(part_number_bootloader);
> +RAVE_SP_DEFINE_SEQ_FILE_PRINT_FUNCTION(copper_rev_deb);
> +RAVE_SP_DEFINE_SEQ_FILE_PRINT_FUNCTION(copper_rev_rmb);
> +RAVE_SP_DEFINE_SEQ_FILE_PRINT_FUNCTION(copper_mod_deb);
> +RAVE_SP_DEFINE_SEQ_FILE_PRINT_FUNCTION(copper_mod_rmb);
> +RAVE_SP_DEFINE_SEQ_FILE_PRINT_FUNCTION(silicon_devid);
> +RAVE_SP_DEFINE_SEQ_FILE_PRINT_FUNCTION(silicon_devrev);

Are these going to debugfs?

								Pavel
								
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

#1723602 — Re: [PATCH v6 1/2] platform: Add driver for RAVE Supervisory Processor

On Wed, Aug 30, 2017 at 3:55 AM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> Add a driver for RAVE Supervisory Processor, an MCU implementing
>> varoius bits of housekeeping functionality (watchdoging, backlight
>> control, LED control, etc) on RAVE family of products by Zodiac
>> Inflight Innovations.
>>
>> This driver implementes core MFD/serdev device as well as
>> communication subroutines necessary for commanding the device.
>
>> diff --git a/Documentation/ABI/testing/sysfs-platform-rave-sp b/Documentation/ABI/testing/sysfs-platform-rave-sp
>> new file mode 100644
>> index 000000000000..81bdc54ba857
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-platform-rave-sp
>> @@ -0,0 +1,35 @@
>> +What:                /sys/devices/platform/*/serial*/serial*-*/reset_reason
>> +Date:                Aug 2017
>> +Contact:     "Andrey Smirnov" <andrew.smirnov@gmail.com>
>> +Description:    (RO) Indicates reason for last reset.
>> +
>> +             The following values can be reported:
>> +                 * 0  -> Normal Power Off
>> +                 * 1  -> Hardware Watchdog
>> +                 * 2  -> Software Watchdog
>> +                 * 3  -> Input Voltage Out Of Range
>> +                 * 4  -> Host Requested
>> +                 * 5  -> Temperature Out Of Range
>> +                 * 6  -> User Requested (via long Power Button press)
>> +                 * 7  -> Illegal Configuration Word
>> +                 * 8  -> Illegal Insturction
>
> Typo here.
>
>> +                 * 9  -> Illegal Trap
>> +                 * 10 -> Unknown
>> +                 * 11 -> Crew Panel Requested
>
> Anyway... If you move management chip to .. I don't know, i2c, the
> path would change. Also it would be different path on N900. Userland
> should not have to deal with this.
>
> And... this should really be string, as the list will need to grow on
> different hardware.
>

I think we have a misunderstanding, with this part of the patch set I
am not trying to propose a generic ABI that would be useful for any
other driver but this one. Hence the lack of concern for different
hardware paths (it's not going to change for this device) and device
specific codes instead of generic strings. I can see how my choice of
generic name such as "reset_reason" might suggest that, so I apologize
for any confusion I might have caused. If said generic name is
unacceptable I can change it to "rave_reset_reason" or something
similar and if that is undesirable as well I am happy to drop this
part of the patch and re-visit this later.

> Plus we'll really need better explanations. What is difference between
> "normal power off" and "host requested"?
>

Short answer: I don't know, since this is as much information that ICD
for that device gave me.

Long answer:  It probably can be discerned from the source code of the
firmware/schematic as well as by bothering the right people, but since
I get a feeling that this attribute is not really desirable in its
current from, I'll punt doing that.

>> +What:                /sys/devices/platform/*/serial*/serial*-*/boot_source
>> +Date:                Aug 2017
>> +Contact:     "Andrey Smirnov" <andrew.smirnov@gmail.com>
>> +Description:    (RW) Indicates currently selected boot source.
>> +
>> +             The following values are valid:
>> +                 * 0  -> SD card
>> +                 * 1  -> eMMC
>> +                 * 2  -> SPI NOR
>> +
>> +             NOTE: Setting boot source on RDU1 hardware is
>> +             currently not implemented
>
> Same comments apply here.
>

Yep, same comment for me as well :-)

>> +RAVE_SP_DEFINE_SEQ_FILE_PRINT_FUNCTION(part_number_firmware);
>> +RAVE_SP_DEFINE_SEQ_FILE_PRINT_FUNCTION(part_number_bootloader);
>> +RAVE_SP_DEFINE_SEQ_FILE_PRINT_FUNCTION(copper_rev_deb);
>> +RAVE_SP_DEFINE_SEQ_FILE_PRINT_FUNCTION(copper_rev_rmb);
>> +RAVE_SP_DEFINE_SEQ_FILE_PRINT_FUNCTION(copper_mod_deb);
>> +RAVE_SP_DEFINE_SEQ_FILE_PRINT_FUNCTION(copper_mod_rmb);
>> +RAVE_SP_DEFINE_SEQ_FILE_PRINT_FUNCTION(silicon_devid);
>> +RAVE_SP_DEFINE_SEQ_FILE_PRINT_FUNCTION(silicon_devrev);
>
> Are these going to debugfs?

They already are there. See rave_sp_debugfs_create() where those
helper functions are being used.

Thanks,
Andrey Smirnov

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

#1723654 — Re: [PATCH v6 1/2] platform: Add driver for RAVE Supervisory Processor

[Multipart message — attachments visible in raw view] — view raw

Hi!

> >> +                 * 9  -> Illegal Trap
> >> +                 * 10 -> Unknown
> >> +                 * 11 -> Crew Panel Requested
> >
> > Anyway... If you move management chip to .. I don't know, i2c, the
> > path would change. Also it would be different path on N900. Userland
> > should not have to deal with this.
> >
> > And... this should really be string, as the list will need to grow on
> > different hardware.
> 
> I think we have a misunderstanding, with this part of the patch set I
> am not trying to propose a generic ABI that would be useful for any
> other driver but this one. Hence the lack of concern for different

Yes, but sorry, that's no-go.  Kernel should hide differences between
different machiens, and it should be rather easy in this case.

> hardware paths (it's not going to change for this device) and device
> specific codes instead of generic strings. I can see how my choice of
> generic name such as "reset_reason" might suggest that, so I apologize
> for any confusion I might have caused. If said generic name is
> unacceptable I can change it to "rave_reset_reason" or something
> similar and if that is undesirable as well I am happy to drop this
> part of the patch and re-visit this later.

Ok, can we do the re-visit thing? You may want to contact these people:

Date: Wed, 30 Aug 2017 12:59:55 +0200
From: Pavel Machek <pavel@ucw.cz>
To: Vadim Pasternak <vadimp@mellanox.com>
Cc: lee.jones@linaro.org, robh+dt@kernel.org,
devicetree@vger.kernel.org,
        j.anaszewski@samsung.com, rpurdie@rpsys.net,
	linux-leds@vger.kernel.org,
	        jiri@resnulli.us, gregkh@linuxfoundation.org,
		platform-driver-x86@vger.kernel.org
Subject: Re: [patch v4 1/2] mfd: Add Mellanox regmap core driver

They are currently trying to add driver, including

>  Cause of reset nodes:
>  - cpu_kernel_panic
>  - cpu_shutdown
>  - bmc_warm_reset

...which sounds very similar to what you are doing.
		
> > Plus we'll really need better explanations. What is difference between
> > "normal power off" and "host requested"?
> >
> 
> Short answer: I don't know, since this is as much information that ICD
> for that device gave me.
> 
> Long answer:  It probably can be discerned from the source code of the
> firmware/schematic as well as by bothering the right people, but since
> I get a feeling that this attribute is not really desirable in its
> current from, I'll punt doing that.

Ok.

> >> +             The following values are valid:
> >> +                 * 0  -> SD card
> >> +                 * 1  -> eMMC
> >> +                 * 2  -> SPI NOR
> >> +
> >> +             NOTE: Setting boot source on RDU1 hardware is
> >> +             currently not implemented
> >
> > Same comments apply here.
> 
> Yep, same comment for me as well :-)

Ok :-).

Best regards,
							Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

#1723970 — Re: [PATCH v6 1/2] platform: Add driver for RAVE Supervisory Processor

[Multipart message — attachments visible in raw view] — view raw

On Thu 2017-08-31 11:10:35, Nikita Yushchenko wrote:
> 
> 
> 30.08.2017 23:38, Pavel Machek wrote:
> > Hi!
> > 
> >>>> +                 * 9  -> Illegal Trap
> >>>> +                 * 10 -> Unknown
> >>>> +                 * 11 -> Crew Panel Requested
> >>>
> >>> Anyway... If you move management chip to .. I don't know, i2c, the
> >>> path would change. Also it would be different path on N900. Userland
> >>> should not have to deal with this.
> >>>
> >>> And... this should really be string, as the list will need to grow on
> >>> different hardware.
> >>
> >> I think we have a misunderstanding, with this part of the patch set I
> >> am not trying to propose a generic ABI that would be useful for any
> >> other driver but this one. Hence the lack of concern for different
> > 
> > Yes, but sorry, that's no-go.  Kernel should hide differences between
> > different machiens, and it should be rather easy in this case.
> 
> There is an interest to have reset reason exported on other ZII hardware
> as well.
> 
> I think that trying to make this generic is purely synthetic. This
> information is board-specific per it's nature, it comes from what board
> is designed for, different boards have quite different sets of possible
> reset reasons. What is needed is - pass this board-specific information
> to board-specific user space.
> 
> What's proper API for that, if not a sysfs attribute?

Please go through the thread.

Sysfs attribute is okay, but:

1) it should probably be a string

2) it should certainly be superset of all the reasons

3) it should be in generic place, say /sys/power/reset_reason

4) it should be documented what each state means
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

#1724018 — Re: [PATCH v6 1/2] platform: Add driver for RAVE Supervisory Processor

>> I think that trying to make this generic is purely synthetic. This
>> information is board-specific per it's nature, it comes from what board
>> is designed for, different boards have quite different sets of possible
>> reset reasons. What is needed is - pass this board-specific information
>> to board-specific user space.
>>
>> What's proper API for that, if not a sysfs attribute?
> 
> Please go through the thread.
> 
> Sysfs attribute is okay, but:
> 
> 1) it should probably be a string
> 
> 2) it should certainly be superset of all the reasons
> 
> 3) it should be in generic place, say /sys/power/reset_reason
> 
> 4) it should be documented what each state means

What I'm concerned here is that a requirement appears for kernel driver
to keep and maintain knowledge of what all that codes mean. For me, it
looks like information locality breakage. Information in question is
application-specific, it definitely changes from board to board and
probably will change for particular board over time.

It's desirable to keep this information application-private, and have
kernel only passed it from hardware to application, unmodified and
uninterpreted.

Requiring kernel driver to interpret application-specific information
only increases complexity (i.e. any changes will have to be maintained
in two places - in driver and in application).

So question is - is there any proper API to communicate
application-private information from hardware through kernel to
userspace without any in-kernel interpretation?

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

#1724022 — Re: [PATCH v6 1/2] platform: Add driver for RAVE Supervisory Processor

[Multipart message — attachments visible in raw view] — view raw

On Thu 2017-08-31 12:01:01, Nikita Yushchenko wrote:
> >> I think that trying to make this generic is purely synthetic. This
> >> information is board-specific per it's nature, it comes from what board
> >> is designed for, different boards have quite different sets of possible
> >> reset reasons. What is needed is - pass this board-specific information
> >> to board-specific user space.
> >>
> >> What's proper API for that, if not a sysfs attribute?
> > 
> > Please go through the thread.
> > 
> > Sysfs attribute is okay, but:
> > 
> > 1) it should probably be a string
> > 
> > 2) it should certainly be superset of all the reasons
> > 
> > 3) it should be in generic place, say /sys/power/reset_reason
> > 
> > 4) it should be documented what each state means
> 
> What I'm concerned here is that a requirement appears for kernel driver
> to keep and maintain knowledge of what all that codes mean. For me,

There's no way around that. Kernel interfaces need to be
documented. If you are passing codes between kernel and application,
_those codes need to be documented_.
 
> So question is - is there any proper API to communicate
> application-private information from hardware through kernel to
> userspace without any in-kernel interpretation?

No.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

#1723984 — Re: [PATCH v6 1/2] platform: Add driver for RAVE Supervisory Processor

30.08.2017 23:38, Pavel Machek wrote:
> Hi!
> 
>>>> +                 * 9  -> Illegal Trap
>>>> +                 * 10 -> Unknown
>>>> +                 * 11 -> Crew Panel Requested
>>>
>>> Anyway... If you move management chip to .. I don't know, i2c, the
>>> path would change. Also it would be different path on N900. Userland
>>> should not have to deal with this.
>>>
>>> And... this should really be string, as the list will need to grow on
>>> different hardware.
>>
>> I think we have a misunderstanding, with this part of the patch set I
>> am not trying to propose a generic ABI that would be useful for any
>> other driver but this one. Hence the lack of concern for different
> 
> Yes, but sorry, that's no-go.  Kernel should hide differences between
> different machiens, and it should be rather easy in this case.

There is an interest to have reset reason exported on other ZII hardware
as well.

I think that trying to make this generic is purely synthetic. This
information is board-specific per it's nature, it comes from what board
is designed for, different boards have quite different sets of possible
reset reasons. What is needed is - pass this board-specific information
to board-specific user space.

What's proper API for that, if not a sysfs attribute?

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

#1724413 — Re: [PATCH v6 1/2] platform: Add driver for RAVE Supervisory Processor

On Mon, Aug 28, 2017 at 09:31:30AM -0700, Andrey Smirnov wrote:
> +static int rave_sp_debugfs_create(struct rave_sp *sp)
> +{
> +	struct dentry *file;
> +
> +	sp->debugfs = debugfs_create_dir("rave", NULL);
> +	if (!sp->debugfs)
> +		return -ENOMEM;

Why do you care about the return value of the debugfs function?  Hint,
you don't...

You should never care if debugfs failed, was built in, or succeeded,
your code should just do the same thing always.  Any value returned from
a debugfs call can just be passed to another one, regardless of the
value returned from the first one.

So here, just save it, and don't check it.

> +	file = debugfs_create_file("i2c_device_status", 0444,
> +				   sp->debugfs, sp,
> +				   &rave_sp_i2c_device_status);
> +	if (!file)
> +		goto error;

Nope, you don't care, just make the call.

> +
> +	file = RAVE_SP_DEBUGFS_CREATE_DEVM_SEQFILE(sp, part_number_firmware);
> +	if (!file)
> +		goto error;

Same for all of these.

> +
> +	file = RAVE_SP_DEBUGFS_CREATE_DEVM_SEQFILE(sp, part_number_bootloader);
> +	if (!file)
> +		goto error;
> +
> +	file = RAVE_SP_DEBUGFS_CREATE_DEVM_SEQFILE(sp, copper_rev_deb);
> +	if (!file)
> +		goto error;
> +
> +	file = RAVE_SP_DEBUGFS_CREATE_DEVM_SEQFILE(sp, copper_rev_rmb);
> +	if (!file)
> +		goto error;
> +
> +	file = RAVE_SP_DEBUGFS_CREATE_DEVM_SEQFILE(sp, copper_mod_deb);
> +	if (!file)
> +		goto error;
> +
> +	file = RAVE_SP_DEBUGFS_CREATE_DEVM_SEQFILE(sp, copper_mod_rmb);
> +	if (!file)
> +		goto error;
> +
> +	file = RAVE_SP_DEBUGFS_CREATE_DEVM_SEQFILE(sp, silicon_devrev);
> +	if (!file)
> +		goto error;
> +
> +	file = RAVE_SP_DEBUGFS_CREATE_DEVM_SEQFILE(sp, silicon_devid);
> +	if (!file)
> +		goto error;
> +
> +	return 0;

And your function can not fail, no need for this to return anything.

> +error:
> +	debugfs_remove_recursive(sp->debugfs);
> +	return -ENOMEM;
> +}
> +
> +static void rave_sp_degugfs_release(struct device *dev, void *res)
> +{
> +	struct rave_sp *sp = *(struct rave_sp **)res;
> +
> +	debugfs_remove_recursive(sp->debugfs);
> +}
> +
> +static int devm_rave_sp_debugfs_create(struct rave_sp *sp)
> +{
> +	struct rave_sp **rcsp;
> +	struct device *dev = &sp->serdev->dev;
> +	int ret;
> +
> +	rcsp = devres_alloc(rave_sp_degugfs_release, sizeof(*rcsp), GFP_KERNEL);
> +	if (!rcsp)
> +		return -ENOMEM;
> +
> +	ret = rave_sp_debugfs_create(sp);
> +	if (!ret) {
> +		*rcsp = sp;
> +		devres_add(dev, rcsp);
> +	} else {
> +		devres_free(rcsp);
> +	}

You should not care what debugfs is doing, if it is working or not.  So
no need to check here either.

debugfs was written to make it dirt-simple to use, I don't know why
people keep trying to put some error handling around it :)

thanks,

greg k-h

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web