Re: Re: [PATCH v3] USB: serial: ftdi_sio: Convert to use dev_groups

Re: Re: [PATCH v3] USB: serial: ftdi_sio: Convert to use dev_groups

[Jiasheng Jiang]

On Fri, Sep 02, 2022 at 10:52:52PM +0800, Greg KH wrote:
>>>>  drivers/usb/serial/ftdi_sio.c | 101 +++++++++++++++++-----------------
>>>>  1 file changed, 51 insertions(+), 50 deletions(-)
>>>> 
>>>> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
>>>> index d5a3986dfee7..479c3a5caaf8 100644
>>>> --- a/drivers/usb/serial/ftdi_sio.c
>>>> +++ b/drivers/usb/serial/ftdi_sio.c
>>>> @@ -1107,11 +1107,40 @@ static u32 ftdi_232bm_baud_base_to_divisor(int baud, int base);
>>>>  static u32 ftdi_232bm_baud_to_divisor(int baud);
>>>>  static u32 ftdi_2232h_baud_base_to_divisor(int baud, int base);
>>>>  static u32 ftdi_2232h_baud_to_divisor(int baud);
>>>> +static umode_t ftdi_sio_attr_is_visible(struct kobject *kobj,
>>>> +					 struct attribute *attr, int idx);
>>>> +static ssize_t latency_timer_store(struct device *dev,
>>>> +				   struct device_attribute *attr,
>>>> +				   const char *valbuf, size_t count);
>>>> +static ssize_t event_char_store(struct device *dev,
>>>> +	struct device_attribute *attr, const char *valbuf, size_t count);
>>>> +static ssize_t latency_timer_show(struct device *dev,
>>>> +				  struct device_attribute *attr, char *buf);
>>>> +
>>> 
>>> Please work with the code so that you do not have to pre-define these
>>> functions.  It should be possible.  Worst case, you pre-define the
>>> structure for the driver, that should be it.
>> 
>> Without pre-definition of the functions, compilation errors will occur,
>> such as 'ftdi_sio_attr_is_visible' undeclared here.
>> I have no idea why they are not necessary.
> 
> If you move the code around that asks for those functions, you will not
> need to define them.
> 

Fine, I have already revised the patch and submitted a v4.

>>> And again, have you tested this change?
>> 
>> Every time I change the code, I recomplie it and check whether there are
>> errors.
>> Are there any other tests I need to do?
> 
> Yes, boot with the device and make sure that the sysfs files are still
> there.  You do have access to one of these devices, right?  They are
> very very common.

Sorry, I still have no idea how to boot with the device.
But if there is any wrong with the patch, you can tell me and I will continue
to revise it.

Thanks,
Jiang

Re: Re: [PATCH v3] USB: serial: ftdi_sio: Convert to use dev_groups

[Johan Hovold]

On Mon, Sep 05, 2022 at 06:01:01PM +0800, Jiasheng Jiang wrote:
> On Fri, Sep 02, 2022 at 10:52:52PM +0800, Greg KH wrote:

> >>> And again, have you tested this change?
> >> 
> >> Every time I change the code, I recomplie it and check whether there are
> >> errors.
> >> Are there any other tests I need to do?
> > 
> > Yes, boot with the device and make sure that the sysfs files are still
> > there.  You do have access to one of these devices, right?  They are
> > very very common.
> 
> Sorry, I still have no idea how to boot with the device.

You need to buy an FTDI device and plug it into a machine with your
patch applied to the running and make sure that you did not break
anything.

> But if there is any wrong with the patch, you can tell me and I will continue
> to revise it.

It is your job to test your patches.

Johan

Re: Re: [PATCH v3] USB: serial: ftdi_sio: Convert to use dev_groups

[Jiasheng Jiang]

On Fri, Sep 02, 2022 at 05:56:13PM +0800, Greg KH wrote:
>>  drivers/usb/serial/ftdi_sio.c | 101 +++++++++++++++++-----------------
>>  1 file changed, 51 insertions(+), 50 deletions(-)
>> 
>> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
>> index d5a3986dfee7..479c3a5caaf8 100644
>> --- a/drivers/usb/serial/ftdi_sio.c
>> +++ b/drivers/usb/serial/ftdi_sio.c
>> @@ -1107,11 +1107,40 @@ static u32 ftdi_232bm_baud_base_to_divisor(int baud, int base);
>>  static u32 ftdi_232bm_baud_to_divisor(int baud);
>>  static u32 ftdi_2232h_baud_base_to_divisor(int baud, int base);
>>  static u32 ftdi_2232h_baud_to_divisor(int baud);
>> +static umode_t ftdi_sio_attr_is_visible(struct kobject *kobj,
>> +					 struct attribute *attr, int idx);
>> +static ssize_t latency_timer_store(struct device *dev,
>> +				   struct device_attribute *attr,
>> +				   const char *valbuf, size_t count);
>> +static ssize_t event_char_store(struct device *dev,
>> +	struct device_attribute *attr, const char *valbuf, size_t count);
>> +static ssize_t latency_timer_show(struct device *dev,
>> +				  struct device_attribute *attr, char *buf);
>> +
> 
> Please work with the code so that you do not have to pre-define these
> functions.  It should be possible.  Worst case, you pre-define the
> structure for the driver, that should be it.

Without pre-definition of the functions, compilation errors will occur,
such as 'ftdi_sio_attr_is_visible' undeclared here.
I have no idea why they are not necessary.
Please explain in detail.

> And again, have you tested this change?

Every time I change the code, I recomplie it and check whether there are
errors.
Are there any other tests I need to do?

Thanks,
Jiang

Re: Re: [PATCH v3] USB: serial: ftdi_sio: Convert to use dev_groups

[Greg KH]

On Fri, Sep 02, 2022 at 10:33:46PM +0800, Jiasheng Jiang wrote:
> On Fri, Sep 02, 2022 at 05:56:13PM +0800, Greg KH wrote:
> >>  drivers/usb/serial/ftdi_sio.c | 101 +++++++++++++++++-----------------
> >>  1 file changed, 51 insertions(+), 50 deletions(-)
> >> 
> >> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> >> index d5a3986dfee7..479c3a5caaf8 100644
> >> --- a/drivers/usb/serial/ftdi_sio.c
> >> +++ b/drivers/usb/serial/ftdi_sio.c
> >> @@ -1107,11 +1107,40 @@ static u32 ftdi_232bm_baud_base_to_divisor(int baud, int base);
> >>  static u32 ftdi_232bm_baud_to_divisor(int baud);
> >>  static u32 ftdi_2232h_baud_base_to_divisor(int baud, int base);
> >>  static u32 ftdi_2232h_baud_to_divisor(int baud);
> >> +static umode_t ftdi_sio_attr_is_visible(struct kobject *kobj,
> >> +					 struct attribute *attr, int idx);
> >> +static ssize_t latency_timer_store(struct device *dev,
> >> +				   struct device_attribute *attr,
> >> +				   const char *valbuf, size_t count);
> >> +static ssize_t event_char_store(struct device *dev,
> >> +	struct device_attribute *attr, const char *valbuf, size_t count);
> >> +static ssize_t latency_timer_show(struct device *dev,
> >> +				  struct device_attribute *attr, char *buf);
> >> +
> > 
> > Please work with the code so that you do not have to pre-define these
> > functions.  It should be possible.  Worst case, you pre-define the
> > structure for the driver, that should be it.
> 
> Without pre-definition of the functions, compilation errors will occur,
> such as 'ftdi_sio_attr_is_visible' undeclared here.
> I have no idea why they are not necessary.

If you move the code around that asks for those functions, you will not
need to define them.

> > And again, have you tested this change?
> 
> Every time I change the code, I recomplie it and check whether there are
> errors.
> Are there any other tests I need to do?

Yes, boot with the device and make sure that the sysfs files are still
there.  You do have access to one of these devices, right?  They are
very very common.

thanks,

greg k-h