Re: [PATCH] ims-pcu: Add commands supported by the new version of the FW

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Fri, Dec 27, 2013 at 08:54:10AM -0800, Andrey Smirnov wrote:
> Sorry for the spam, sent the first version of the reply in non plain/text.
> 
> >> +static ssize_t ims_pcu_ofn_reg_data_show(struct device *dev,
> >> +                                      struct device_attribute *dattr,
> >> +                                      char *buf)
> >> +{
> >> +     struct usb_interface *intf = to_usb_interface(dev);
> >> +     struct ims_pcu *pcu = usb_get_intfdata(intf);
> >> +     int error;
> >> +
> >> +     mutex_lock(&pcu->cmd_mutex);
> >> +
> >> +     error = ims_pcu_execute_command(pcu, OFN_GET_CONFIG,
> >> +                                     &pcu->ofn_reg_addr,
> >> +                                     sizeof(pcu->ofn_reg_addr));
> >> +     if (error >= 0) {
> >> +             const s16 result = pcu->cmd_buf[OFN_REG_RESULT_MSB_OFFSET] << 8 |
> >> +                                pcu->cmd_buf[OFN_REG_RESULT_LSB_OFFSET];
> >
> > const here seems overkill.
> 
> That variable has a lifetime limited by the scope of if statement and
> during that lifetime its value it constant, so I'm not sure I
> understand what you mean by "overkill"?

I usually only bother with const when dealing with pointers and rarely
if ever for non-pointer arguments and local variables. That style mostly
matches the rest of the kernel.

[...]

> 
> >
> >> +             error = result;
> >
> > Does firmware guarantee that it would return errors that make sense to
> > Linux?
> 
> Firmware returns -ENOENT.

OK, still, I'd feel safer if we did something like

	if (result != 0)
		error = -EIO; /* or -ENOENT */

This way we can be sure that even if subsequent firmware version change
error codes for some reason we'd still be passing to the upper layer
Linux-specific ones.

[...]

> >>       if (error)
> >> +             goto err_stop_io;
> >> +
> >> +     error = sysfs_create_group(&intf->dev.kobj, &ims_pcu_attr_group);
> >> +     if (error)
> >>               goto err_remove_sysfs;
> >
> > Why did you move sysfs group creation? Now one can not observe progress
> > of firmware update...
> 
> I need the device ID to be known before the sysfs group is created in
> order to make the decision about OFN-related attributes visibility,
> and for that I need "ims_pcu_init_application_mode" to be called
> before "sysfs_create_group" call is made.
> 
> I see two potential ways of solving this problem:
> 
>  1. Split the calls to "ims_pcu_init_bootlader_mode" and
> "ims_pcu_init_application_mode" and make the former after the group is
> created and the latter before.
>  2. Remove the "update_firmware_status" attribute (Does the userspace
> in the system that uses this driver actually rely on this argument or
> is just added for convenience? I know that they have a userspace tool
> that they use to update the FW, so that's why I am wondering if they
> ever use it to monitor the progress)

The update_firmware_status attribute was a requirement from IMS. As far
as I know the flashing is done with standard udev facilities, so the
status is there to see what happened if something goes wrong. Still, I
believe we have to keep it.

I guess we can split the calls but even then ims_pcu_identify_type() may
fail and leave you with garbage device_id. Maybe we should alter
ims_pcu_is_attr_visible() to do:

	if (pcu->bootloader_mode) {
		...
	} else {
		if (pcu->setup_complete) {
			check_if_ofn_attrs_should_be_visible;
		}
	}

and then do 

	ysfs_update_group(&pcu->dev->kobj, &ims_pcu_attr_group);

at the end of ims_pcu_init_application_mode().

Thanks.

-- 
Dmitry