On Mon, Aug 10, 2020 at 05:48:07PM -0500, David Lechner wrote: > > >>>>> > >>>>> CPMAC ETHERNET DRIVER > >>>>> M: Florian Fainelli <f.faine...@gmail.com> > >>>>> 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) > { > ... > }
Regardless of endianness for pointers, will targets that have -fshort-enums enabled by default present a problem here? I imagine that in these cases enum some_type will have a size of unsigned char because that is the first type that can represent all the values: https://gcc.gnu.org/onlinedocs/gcc/Structures-unions-enumerations-and-bit-fields-implementation.html What I'm worried about is whether we can gurantee u32 val can be swapped out with enum some_type val -- or if this is not possible because some architectures will be built with -fshort-enums enabled? William Breathitt Gray
signature.asc
Description: PGP signature