Re: [PATCH 3/3] iio: Add Qualcomm Sensor Manager drivers

[Jonathan Cameron <jic23@kernel.org>]

> >> +
> >> +static void qcom_smgr_accel_remove(struct platform_device *pdev)  
> > 
> > I'm surprised to see a platform device here - will read on but I
> > doubt that is the way to go.  Maybe an auxbus or similar or
> > just squashing this all down to be registered directly by
> > the parent driver.  
> I got the idea from cros_ec_sensors which also deals with a similar 
> sensor hub paradigm.

Generally the use of platform drivers for subfunctions of something
is now not considered the way to go.  Here there seems to be little
point in spinning out another layer of devices.
> >   
> >> +static void qcom_smgr_buffering_report_handler(struct qmi_handle *hdl,
> >> +					       struct sockaddr_qrtr *sq,
> >> +					       struct qmi_txn *txn,
> >> +					       const void *data)
> >> +{
> >> +	struct qcom_smgr *smgr =
> >> +		container_of(hdl, struct qcom_smgr, sns_smgr_hdl);
> >> +	struct sns_smgr_buffering_report_ind *ind =
> >> +		(struct sns_smgr_buffering_report_ind *)data;  
> > 
> > Casting away a const isn't a good sign. Why do you need to do that?
> > 	const struct sns_smg_buffer_repor_ind *ind = data;
> > should be fine I think.  
> 
> The casted struct was previously not const so I was only casting from 
> void *. I made it const lately but didn't notice this cast. Will change it.

Ok. But never a reason to cast from a void *.  The C spec says that
happens implicitly just fine.

> >> +	ret = qcom_smgr_request_all_sensor_info(smgr, &smgr->sensors);
> >> +	if (ret < 0) {
> >> +		dev_err(smgr->dev, "Failed to get available sensors: %pe\n",
> >> +			ERR_PTR(ret));
> >> +		return ret;
> >> +	}
> >> +	smgr->sensor_count = ret;
> >> +
> >> +	/* Get primary and secondary sensors from each sensor ID */
> >> +	for (i = 0; i < smgr->sensor_count; i++) {
> >> +		ret = qcom_smgr_request_single_sensor_info(smgr,
> >> +							   &smgr->sensors[i]);
> >> +		if (ret < 0) {
> >> +			dev_err(smgr->dev,
> >> +				"Failed to get sensors from ID 0x%02x: %pe\n",
> >> +				smgr->sensors[i].id, ERR_PTR(ret));
> >> +			return ret;
> >> +		}
> >> +
> >> +		for (j = 0; j < smgr->sensors[i].data_type_count; j++) {
> >> +			/* Default to maximum sample rate */
> >> +			smgr->sensors[i].data_types->cur_sample_rate =
> >> +				smgr->sensors[i].data_types->max_sample_rate;
> >> +
> >> +			dev_dbg(smgr->dev, "0x%02x,%d: %s %s\n",
> >> +				smgr->sensors[i].id, j,
> >> +				smgr->sensors[i].data_types[j].vendor,
> >> +				smgr->sensors[i].data_types[j].name);
> >> +		}
> >> +
> >> +		qcom_smgr_register_sensor(smgr, &smgr->sensors[i]);  
> > Above I suggest that maybe you should just skip the platform devices and register
> > directly with IIO as you find the sensors. So have the struct iio_dev->device
> > parent directly off this one.  
> 
> As I said previously I followed the model used in cros_ec_sensors, and 
> it made sense to me since I always see platform devices used to 
> represent firmware-backed devices like this.

In this case you end up with

parent device
    |
    |____________________
    |         |          |
ChildA     ChildB       ChildC  (all platform devices)
    |         |          |
IIODevA    IIODevB      IIODEVC

Today we'd probably do those child devices using auxiliary devices but
aside from that, the only reason to do this is you want to have separate
drivers for each child.

You can just do

parent device
   |
   |_______________________
   |         |             |
IIODevA    IIODevB       IIODevC

for the case where there no separate child drivers.

That is the parent driver can just instantiate the IIO Bus Devices
directly (it's kind of a Class really but a bus for historical reasons).
Various IMUs do this when they have separately controlled sampling frequencies
for different types of sensor or even separate fifos. (They are really
sensor hubs, just connected of SPI or similar).

So it's up to you whether you want the separate per channel type devices and
to handle the potential races around those going away.
qcom_smgr_buffering_report_handler() for instance needs a lock to
stop the child driver being unbound between the checks on iio_dev and the
use of it. With one driver that complexity doesn't occur.
See for example drivers/iio/st_lsm6dsx/ that does it this way.

Also possible would be a single IIO device with multiple buffers.
We only have a few of those though so you might run into missing bits of ABI
taking that path.

Jonathan