Re: [PATCH v3 2/4] drivers: pwm: pwm-cros-ec: add channel type support

[Fabio Baltieri]

Hi Tzung-Bi,

On Tue, Apr 12, 2022 at 03:32:01PM +0800, Tzung-Bi Shih wrote:
> On Mon, Apr 11, 2022 at 03:21:12PM +0000, Fabio Baltieri wrote:
> > Add support for EC_PWM_TYPE_DISPLAY_LIGHT and EC_PWM_TYPE_KB_LIGHT pwm
> > types to the PWM cros_ec_pwm driver. This allows specifying one of these
> > PWM channel by functionality, and let the EC firmware pick the correct
> > channel, thus abstracting the hardware implementation from the kernel
> > driver.
> > 
> > To use it, define the node with the "google,cros-ec-pwm-type"
> > compatible.
> 
> Not sure whether you decide to leave the prefix as is or not[1], just another
> reminder: to be neat, suggest to remove "drivers: " prefix from the commit
> title.

Sorry I messed up there, fixed in on the wrong branch and then forgot to
re-fix it once I moved to the wrong one -- I'll make sure to get that
right now, thanks for your patience.

> 
> [1]: https://patchwork.kernel.org/project/chrome-platform/patch/20220331125818.3776912-3-fabiobaltieri@xxxxxxxxxxxx/
> 
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id cros_ec_pwm_of_match[] = {
> > +	{
> > +		.compatible = "google,cros-ec-pwm",
> > +	},
> > +	{
> > +		.compatible = "google,cros-ec-pwm-type",
> > +		.data = OF_CROS_EC_PWM_TYPE,
> > +	},
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, cros_ec_pwm_of_match);
> > +#else
> > +#define cros_ec_pwm_of_match NULL
> > +#endif
> > +
> >  static int cros_ec_pwm_probe(struct platform_device *pdev)
> >  {
> >  	struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
> >  	struct device *dev = &pdev->dev;
> > +	const struct of_device_id *id;
> >  	struct cros_ec_pwm_device *ec_pwm;
> >  	struct pwm_chip *chip;
> >  	int ret;
> > @@ -251,17 +312,27 @@ static int cros_ec_pwm_probe(struct platform_device *pdev)
> >  	chip = &ec_pwm->chip;
> >  	ec_pwm->ec = ec;
> >  
> > +	id = of_match_device(cros_ec_pwm_of_match, dev);
> > +	if (id && id->data == OF_CROS_EC_PWM_TYPE)
> > +		ec_pwm->use_pwm_type = true;
> > +
> [...]
> > -#ifdef CONFIG_OF
> > -static const struct of_device_id cros_ec_pwm_of_match[] = {
> > -	{ .compatible = "google,cros-ec-pwm" },
> > -	{},
> > -};
> > -MODULE_DEVICE_TABLE(of, cros_ec_pwm_of_match);
> > -#endif
> > -
> 
> Use dev->driver->of_match_table to access the table so that the table
> declaration doesn't actually need a move.  Instead, the helper function
> of_device_get_match_data() is preferred.
> 
> Alternatively, it could use
> of_device_is_compatible(..."google,cros-ec-pwm-type") so that it doesn't
> need to introduce OF_CROS_EC_PWM_TYPE and reduce some bits.  I would prefer
> this way.

That's a very good point, should simplify things a fair bit, did not
realize of_device_is_compatible() was an option somehow, will rework it
for that and send a v4 soon. Thanks for the pointer.

Fabio

-- 
Fabio Baltieri