Re: [PATCH v4 1/5] counter: Internalize sysfs interface code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




CPMAC ETHERNET DRIVER
    M:	Florian Fainelli <f.fainelli@xxxxxxxxx>
diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c
index 78766b6ec271..0f20920073d6 100644
--- a/drivers/counter/104-quad-8.c
+++ b/drivers/counter/104-quad-8.c
@@ -621,7 +621,7 @@ static const struct iio_chan_spec quad8_channels[] = {
    };
static int quad8_signal_read(struct counter_device *counter,
-	struct counter_signal *signal, enum counter_signal_value *val)
+			     struct counter_signal *signal, u8 *val)

I'm not a fan of replacing enum types with u8 everywhere in this patch.
But if we have to for technical reasons (e.g. causes compiler error if
we don't) then it would be helpful to add comments giving the enum type
everywhere like this instance where u8 is actually an enum value.

If we use u32 as the generic type for enums instead of u8, I think the
compiler will happlily let us use enum type and u32 interchangeably and
not complain.

I switched to fixed-width types after the suggestion by David Laight:
https://lkml.org/lkml/2020/5/3/159. I'll CC David Laight just in case he
wants to chime in again.

Enum types would be nice for making the valid values explicit, but there
is one benefit I have appreciated from the move to fixed-width types:
there has been a significant reduction of duplicate code; before, we had
a different read function for each different enum type, but now we use a
single function to handle them all.

Yes, what I was trying to explain is that by using u32 instead of u8, I
think we can actually do both.

The function pointers in struct counter_device *counter would use u32 as a
generic enum value in the declaration, but then the actual implementations
could still use the proper enum type.

Oh, I see what you mean now. So for example:

     int (*signal_read)(struct counter_device *counter,
                        struct counter_signal *signal, u8 *val)

This will become instead:

     int (*signal_read)(struct counter_device *counter,
                        struct counter_signal *signal, u32 *val)

Then in the driver callback implementation we use the enum type we need:

     enum counter_signal_level signal_level = COUNTER_SIGNAL_HIGH;
     ...
     *val = signal_level;

Is that what you have in mind?


Yes.

Additionally, if we have...


      int (*x_write)(struct counter_device *counter,
                     ..., u32 val)
We should be able to define the implementation as:

static int my_driver_x_write(struct counter_device *counter,
                             ..., enum some_type val)
{
	...
}

Not sure if it works if val is a pointer though. Little-
endian systems would probably be fine, but maybe not big-
endian combined with -fshort-enums compiler flag.


      int (*x_read)(struct counter_device *counter,
                    ..., u32 *val)
static int my_driver_x_read(struct counter_device *counter,
                            ..., enum some_type *val)
{
	...
}




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux