Re: [PATCH v2 03/27] coresight: Add helper device type

[Suzuki K Poulose]

On 05/03/2018 06:00 PM, Mathieu Poirier wrote:

...

> > +/*
> > + * coresight_release_device - Release this device and any of the helper
> > + * devices connected to it for trace operation.
> > + */
> > +static void coresight_release_device(struct coresight_device *csdev)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < csdev->nr_outport; i++) {
> > +		struct coresight_device *child = csdev->conns[i].child_dev;
> > +
> > +		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
> > +			pm_runtime_put(child->dev.parent);
> > +	}
>
> There is a newline here in coresight_prepare_device().  Either add one (or not)
> in both function but please be consistent.

> > @@ -480,8 +517,7 @@ static int _coresight_build_path(struct coresight_device *csdev,
> >   
> >   	node->csdev = csdev;
> >   	list_add(&node->link, path);
> > -	pm_runtime_get_sync(csdev->dev.parent);
> > -
> > +	coresight_prepare_device(csdev);
>
> There was a newline between pm_runtime_get_sync() and the return statement in
> the original code.


> > @@ -775,6 +811,10 @@ static struct device_type coresight_dev_type[] = {
> >   		.name = "source",
> >   		.groups = coresight_source_groups,
> >   	},
> > +	{
> > +		.name = "helper",
> > +	},
> > +
>
> Extra newline.

> >   };   
> > +/**
> > + * struct coresight_ops_helper - Operations for a helper device.
> > + *
> > + * All operations could pass in a device specific data, which could
> > + * help the helper device to determine what to do.
> > + *
> > + * @enable	: Turn the device ON.
> > + * @disable	: Turn the device OFF.
>
> There is a discrepancy between the comment and the operations, i.e enabling a
> device is not synonymous of turning it on.  Looking at patch 04/27 the ops is
> called in tmc_etr_enable/disable_catu() so the comment propably needs to be
> changed.

Sure, will fix all of them.

Cheers
Suzuki