| Message ID | 20201103072104.12361-2-mdf@kernel.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | DFL Module support | expand |
On Mon, Nov 02, 2020 at 11:21:01PM -0800, Moritz Fischer wrote: > From: Xu Yilun <yilun.xu@intel.com> > > The value of the field dfl_device.type comes from the 12 bits register > field DFH_ID according to DFL spec. So this patch changes the definition > of the type field to u16. > > Also it is not necessary to illustrate the valid bits of the type field > in comments. Instead we should explicitly define the possible values in > the enumeration type for it, because they are shared by hardware spec. > We should not let the compiler decide these values. > > Similar changes are also applied to dfl_device.feature_id. > > This patch also fixed the MODALIAS format according to the changes > above. > > Signed-off-by: Xu Yilun <yilun.xu@intel.com> > Reviewed-by: Tom Rix <trix@redhat.com> > Signed-off-by: Moritz Fischer <mdf@kernel.org> > --- > drivers/fpga/dfl.c | 3 +-- > drivers/fpga/dfl.h | 14 +++++++------- > 2 files changed, 8 insertions(+), 9 deletions(-) > > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c > index b450870b75ed..5a6ba3b2fa05 100644 > --- a/drivers/fpga/dfl.c > +++ b/drivers/fpga/dfl.c > @@ -298,8 +298,7 @@ static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env) > { > struct dfl_device *ddev = to_dfl_dev(dev); > > - /* The type has 4 valid bits and feature_id has 12 valid bits */ > - return add_uevent_var(env, "MODALIAS=dfl:t%01Xf%03X", > + return add_uevent_var(env, "MODALIAS=dfl:t%04Xf%04X", Why drop the comment and not just fix it up instead? > ddev->type, ddev->feature_id); > } > > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h > index 5dc758f655b7..ac373b1fcff9 100644 > --- a/drivers/fpga/dfl.h > +++ b/drivers/fpga/dfl.h > @@ -520,19 +520,19 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev, > * enum dfl_id_type - define the DFL FIU types > */ > enum dfl_id_type { > - FME_ID, > - PORT_ID, > + FME_ID = 0, > + PORT_ID = 1, > DFL_ID_MAX, > }; > > /** > * struct dfl_device_id - dfl device identifier > - * @type: contains 4 bits DFL FIU type of the device. See enum dfl_id_type. > - * @feature_id: contains 12 bits feature identifier local to its DFL FIU type. > + * @type: DFL FIU type of the device. See enum dfl_id_type. > + * @feature_id: feature identifier local to its DFL FIU type. > * @driver_data: driver specific data. > */ > struct dfl_device_id { > - u8 type; > + u16 type; Why isn't this enum dfl_id_type? > u16 feature_id; > unsigned long driver_data; > }; > @@ -543,7 +543,7 @@ struct dfl_device_id { > * @dev: generic device interface. > * @id: id of the dfl device. > * @type: type of DFL FIU of the device. See enum dfl_id_type. > - * @feature_id: 16 bits feature identifier local to its DFL FIU type. > + * @feature_id: feature identifier local to its DFL FIU type. But feature_id is still 16 bits, why change this? > * @mmio_res: mmio resource of this dfl device. > * @irqs: list of Linux IRQ numbers of this dfl device. > * @num_irqs: number of IRQs supported by this dfl device. > @@ -553,7 +553,7 @@ struct dfl_device_id { > struct dfl_device { > struct device dev; > int id; > - u8 type; > + u16 type; why isn't this an enum as well? greg k-h
Hi Greg: On Tue, Nov 03, 2020 at 08:44:52AM +0100, Greg KH wrote: > On Mon, Nov 02, 2020 at 11:21:01PM -0800, Moritz Fischer wrote: > > From: Xu Yilun <yilun.xu@intel.com> > > > > The value of the field dfl_device.type comes from the 12 bits register > > field DFH_ID according to DFL spec. So this patch changes the definition > > of the type field to u16. > > > > Also it is not necessary to illustrate the valid bits of the type field > > in comments. Instead we should explicitly define the possible values in > > the enumeration type for it, because they are shared by hardware spec. > > We should not let the compiler decide these values. > > > > Similar changes are also applied to dfl_device.feature_id. > > > > This patch also fixed the MODALIAS format according to the changes > > above. > > > > Signed-off-by: Xu Yilun <yilun.xu@intel.com> > > Reviewed-by: Tom Rix <trix@redhat.com> > > Signed-off-by: Moritz Fischer <mdf@kernel.org> > > --- > > drivers/fpga/dfl.c | 3 +-- > > drivers/fpga/dfl.h | 14 +++++++------- > > 2 files changed, 8 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c > > index b450870b75ed..5a6ba3b2fa05 100644 > > --- a/drivers/fpga/dfl.c > > +++ b/drivers/fpga/dfl.c > > @@ -298,8 +298,7 @@ static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env) > > { > > struct dfl_device *ddev = to_dfl_dev(dev); > > > > - /* The type has 4 valid bits and feature_id has 12 valid bits */ > > - return add_uevent_var(env, "MODALIAS=dfl:t%01Xf%03X", > > + return add_uevent_var(env, "MODALIAS=dfl:t%04Xf%04X", > > Why drop the comment and not just fix it up instead? Previously in Pull request for 5.10, you suggested that don't care too much about the exact data width. "Who cares it's only 4 bits, we have room in the kernel to have it be a full 8 bits or 32 or whatever" So I changed the type of 'type' & 'feature_id' all to u16 and deleted all the comments about the width of them. > > > ddev->type, ddev->feature_id); > > } > > > > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h > > index 5dc758f655b7..ac373b1fcff9 100644 > > --- a/drivers/fpga/dfl.h > > +++ b/drivers/fpga/dfl.h > > @@ -520,19 +520,19 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev, > > * enum dfl_id_type - define the DFL FIU types > > */ > > enum dfl_id_type { > > - FME_ID, > > - PORT_ID, > > + FME_ID = 0, > > + PORT_ID = 1, > > DFL_ID_MAX, > > }; > > > > /** > > * struct dfl_device_id - dfl device identifier > > - * @type: contains 4 bits DFL FIU type of the device. See enum dfl_id_type. > > - * @feature_id: contains 12 bits feature identifier local to its DFL FIU type. > > + * @type: DFL FIU type of the device. See enum dfl_id_type. > > + * @feature_id: feature identifier local to its DFL FIU type. > > * @driver_data: driver specific data. > > */ > > struct dfl_device_id { > > - u8 type; > > + u16 type; > > Why isn't this enum dfl_id_type? We are going to move the two fields to mod_devicetable.h, they will be eventually changed to __u16, so I use u16 here. > > > u16 feature_id; > > unsigned long driver_data; > > }; > > @@ -543,7 +543,7 @@ struct dfl_device_id { > > * @dev: generic device interface. > > * @id: id of the dfl device. > > * @type: type of DFL FIU of the device. See enum dfl_id_type. > > - * @feature_id: 16 bits feature identifier local to its DFL FIU type. > > + * @feature_id: feature identifier local to its DFL FIU type. > > But feature_id is still 16 bits, why change this? As I mentioned before, I mean not to emphasize on the bitfield width. > > > > * @mmio_res: mmio resource of this dfl device. > > * @irqs: list of Linux IRQ numbers of this dfl device. > > * @num_irqs: number of IRQs supported by this dfl device. > > @@ -553,7 +553,7 @@ struct dfl_device_id { > > struct dfl_device { > > struct device dev; > > int id; > > - u8 type; > > + u16 type; > > why isn't this an enum as well? Same as before, the 'type' should be sync with dfl_device_id.type. Thanks, Yilun > > greg k-h
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c index b450870b75ed..5a6ba3b2fa05 100644 --- a/drivers/fpga/dfl.c +++ b/drivers/fpga/dfl.c @@ -298,8 +298,7 @@ static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env) { struct dfl_device *ddev = to_dfl_dev(dev); - /* The type has 4 valid bits and feature_id has 12 valid bits */ - return add_uevent_var(env, "MODALIAS=dfl:t%01Xf%03X", + return add_uevent_var(env, "MODALIAS=dfl:t%04Xf%04X", ddev->type, ddev->feature_id); } diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h index 5dc758f655b7..ac373b1fcff9 100644 --- a/drivers/fpga/dfl.h +++ b/drivers/fpga/dfl.h @@ -520,19 +520,19 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev, * enum dfl_id_type - define the DFL FIU types */ enum dfl_id_type { - FME_ID, - PORT_ID, + FME_ID = 0, + PORT_ID = 1, DFL_ID_MAX, }; /** * struct dfl_device_id - dfl device identifier - * @type: contains 4 bits DFL FIU type of the device. See enum dfl_id_type. - * @feature_id: contains 12 bits feature identifier local to its DFL FIU type. + * @type: DFL FIU type of the device. See enum dfl_id_type. + * @feature_id: feature identifier local to its DFL FIU type. * @driver_data: driver specific data. */ struct dfl_device_id { - u8 type; + u16 type; u16 feature_id; unsigned long driver_data; }; @@ -543,7 +543,7 @@ struct dfl_device_id { * @dev: generic device interface. * @id: id of the dfl device. * @type: type of DFL FIU of the device. See enum dfl_id_type. - * @feature_id: 16 bits feature identifier local to its DFL FIU type. + * @feature_id: feature identifier local to its DFL FIU type. * @mmio_res: mmio resource of this dfl device. * @irqs: list of Linux IRQ numbers of this dfl device. * @num_irqs: number of IRQs supported by this dfl device. @@ -553,7 +553,7 @@ struct dfl_device_id { struct dfl_device { struct device dev; int id; - u8 type; + u16 type; u16 feature_id; struct resource mmio_res; int *irqs;